(Best Practice) Keys in assoz. Array einfügen und entfernen

[cassidy]

Hallo,

ich möchte als Übungsprojekt einen kleinen Shop basteln.

Den Einkaufskorb bilde ich über ein Array ab.
Der Key ist dabei die Produkt-ID, der Value die Anzahl der Waren im Korb.
Wird ein neues Produkt hinzugefügt, möchte ich zunächst schauen, ob die Ware schon vorhanden ist. Wenn ja, soll einfach der Zähler erhöht werden, wenn nein soll ein neuer Key angelegt werden.

Beim Löschen möchte ich den jeweiligen Key entfernen, sobald der Value 0 ist, sobald also keine Waren mehr im Korb sind.

Mein Code:

/**
 * provides functions to edit users shopping cart
 * and calculate final amount to pay
 */
class CartService{
    /**
     * adds product to users cart
         * creates new key-value-pair when cart has no matching product
     * @param User $user
     * @param Product $product
     * @return new cart
     */
    public function add(User $user,Product $product){
        $cart = $user->getShoppingCart();
        if(array_key_exists($product->getID(), $cart)){
            $count = $cart[$product->getID()];
            $cart[$product->getID()] = $count+1;
        } else {
            $cart[$product->getID()] = 1;
        }
        $user->setShoppingCart($cart);
        return $cart;
    }

    /**
     * deletes product from users cart    
         * deletes key-value-pair when number of products gets zero 
     * @param User $user
     * @param Product $product
     * @return new cart
     */
    public function delete(User $user,Product $product){
        $cart = $user->getShoppingCart();
        $count = $cart[$product->getID()];
        $count -= 1;
        if($count == 0){
            unset($count[$product->getID()]);
        } else {
            $cart[$product->getID()] = $count;
        }
        $user->setShoppingCart($cart);
        return $cart;
    }
} 

Für mich sieht das aber alles ziemlich ... zusammen gefrickelt aus.
Kann man das irgendwie besser / schöner schreiben?

[mermshaus]

Man könnte das vielleicht hier und da etwas klarer schreiben. Zum Beispiel:

    public function add(User $user,Product $product){
        $cart = $user->getShoppingCart();
        $id   = $product->getID();

        if(false === array_key_exists($id, $cart)){
            $cart[$id] = 1;
        } else {
            $cart[$id]++;
        }

        $user->setShoppingCart($cart);
    } 

    public function delete(User $user,Product $product){
        $cart = $user->getShoppingCart();
        $id   = $product->getID();

        if(1 === $cart[$id]){
            unset($cart[$id]);
        } else {
            $cart[$id]--;
        }

        $user->setShoppingCart($cart);
    } 

(Ich sollte dabei auch noch ein, zwei Fehler korrigiert haben.)

Die return-Zeilen habe ich entfernt, weil die meines Erachtens keinen Sinn ergeben.

Die eigentlich Frage zu dem Code dürfte aber sein: Warum legst du Cart nicht als Klasse an?

[cassidy]

Danke, das ist wirklich übersichtlicher.

Ich brauche genau zwei Informationen: Welches Produkt und wie viele davon. Ein eigene Klasse fand ich dafür n bisschen oversized, lasse mich aber auch gern eines Besseren belehren.

[mermshaus]

Das würde es dir zum Beispiel ersparen, den Cart bei Änderung neu zuweisen zu müssen, und du hättest Type-Hinting und Sichtbarkeiten und andere OOP-Vorteile.

Ich hätte das aber vermutlich ohnehin eher so implementiert:

class Cart
{
    public function add(Product $product, $quantity) {}
    public function remove(Product $product, $quantity) {}
}

class User
{
    public function setCart(Cart $cart);
    public function getCart();
} 

Aber keine Ahnung, ist deine Entscheidung.

[jack88]

class Cart
{
public function add(Product $product, $quantity) {}
public function remove(Product $product, $quantity) {}
}

Halte ich für keine so gute Idee.... sowas führt schnell in eine konzeptionelle Sackgasse.

Ein Schuh z.B. KANN in der Größe 39-45 und in der Farbe schwarz oder braun bestellt werden. Im Warenkorb HAT er die Schuhgröße 39 und die Farbe Schwarz. Ein Warenkorbartikel unterscheidet sich i.d.R. z.B. durch die konkrete vom Kunden getroffene Auswahl und ggf. auch durch weitere Kundenwünsche, Bemerkungen etc. von einem Artikel auf der Angebotsseite.

Diese Problematik läßt sich mit einer Methode wie cart->add(Product $product, quantity) nicht vernünftig abbilden.

Deshalb würde ich prinzipiell immer zwischen einem Artikel und einem Warenkorbartikel unterscheiden und zu einem Ansatz wie diesem tendieren:

class Cart
{
    public function add(CartItemInterface $cartItem) {}
    public function remove(CartItemInterface $cartItem, $quantity = null) {}
}

interface CartItemInterface {
    public function getProduct();
    public function getProductAttributes();
    public function getPrice();
    public function getQuantity();
}

class CartItem implements CartItemInterface {
    /* @var Product */
    private $product = null;
    private $quantity = 0;
    /* @var AttributesCollection */
    private $attributes = array();

    public function __construct(Product $product, $quantity, AttributesCollection $attributes = array()) {
        $this->product = $product;
        $this->quantity = $quantity;
        $this->attributes = $attributes;
    }

    public function getProduct() {
        return $this->product;
    }
    public function getPrice() {
        $attributesPrice = $this->getAttributesPrice();
        $productPrice = $this->getProduct()->getPrice();
        return ($attributesPrice + $productPrice) * $this->getQuantity();
    }

    public function getQuantity() {
        $this->quantity;
    }

    public function getProductAttributes() {
        return $this->attributes;
    }

    protected function getAttributesPrice() {
        $price = 0;
        foreach($this->getProductAttributes() as $attribute) {
            $price += $attribute->getPrice();
        }
        return $price;
    }
} 

class User
{
public function setCart(Cart $cart);
public function getCart(Cart $cart);
}

Auch das halte ich konzeptionell für keine gute Idee. Seit wann ist denn ein Warenkorb eine Usereigenschaft und wo würde das hinführen:

 $user->setCart(...);
$user->setAddress(...);
$user->setOrders(...);
… 

kann so nicht der richtige Ansatz sein. Klar zwischen User und Warenkorb besteht eine Beziehung, aber warum muss diese unbedingt in einem der beteiligten Objekte abgebildet werden?

Warum nicht so:

$cart = $cartService->getCartByUser($user);
$address = $addressService->getAddressByUser($user);
$orders = $orderService->getOrdersByUser($user); 

vg
jack

[erc]

Ein Schuh z.B. KANN in der Größe 39-45 und in der Farbe schwarz oder braun bestellt werden. Im Warenkorb HAT er die Schuhgröße 39 und die Farbe Schwarz. Ein Warenkorbartikel unterscheidet sich i.d.R. z.B. durch die konkrete vom Kunden getroffene Auswahl und ggf. auch durch weitere Kundenwünsche, Bemerkungen etc. von einem Artikel auf der Angebotsseite.

Da kann man sich jetzt aber auch drüber streiten ob "Schuh, Größe 39-45, schwarz oder braun" ein Produkt ist oder doch 12 Verschiedene. Der erstere Ansatz würde ausserdem bei der original implementierung dazu führen das der Kunde nie unterschiedliche größen oder farben bestellen könnte.

Ich stimm dir aber auf jedenfall zu, dass das so ziemlich kurzsichtig ist. Eine Datensturktur und ein Interface was eine Warenkorbposition nur mit zwei Eigenschaften spezifizieren kann... eher nicht so eine gut Idee.

[jack88]

Da kann man sich jetzt aber auch drüber streiten ob "Schuh, Größe 39-45, schwarz oder braun" ein Produkt ist oder doch 12 Verschiedene.�*

Das stimmt. Es kommt auch auf die Produkte an. Schuhe könnte man durchaus so in allen möglichen Variationen noch anbieten, aber wenn man z.B. einen Pizzashop betreibt, dann möchte man jede Pizza mit allen erdenklichen Extras bestellen können. In diesem Fall wäre der Ansatz kaum realisierbar bzw. kaum praktikabel. Deswegen würde ich prinzipiell von dem Ansatz abraten, da er einen nur unnötig einschränkt und sonst keine Vorteile mitbringt.


@cassidy
Versuche zuerst die Verantwortlichkeiten zu klären und besser zu trennen, überlege Dir genau welche Aufgaben die jeweilige Komponente übernehmen soll und welche nicht. Z.B.:

- Item => Repräsentiert einen Artikel (Name, Preis, verfügbare Optionen etc.)
- CartItem => Repräsentiert Artikel im Warenkorb (gewählte Optionen, Menge)
- Cart => Verwaltet Warenkorbartikel
- CartService => Verwaltet Warenkörbe

interface CartServiceInterface {
    public function addCart(Cart $cart, User $user = null );
    public function findAll();
    public function findCartByUser(User $user);
    public function deleteCartByUser(User $user);
    public function deleteCarts();;
    public function countCarts();
    //.. usw.
}

interface CartInterface {
    public function add(CartItemInterface $cartItem);
    public function remove(CartItemInterface $cartItem, $quantity = null);
    public function get(CartItemInterface $cartItem = null);
    public function getItemByPosition($position);
    public function getItemByNumber($itemnumber);
    public function getItemByName($name);
    public function countItems();
    // etc...
}

interface ItemInterface {
    public function getName();
    public function getDescription();
    public function getOptions();
    public function getPrice();
}

interface CartItemInterface {
    public function getProduct();
    public function getProductAttributes();
    public function getPrice();
    public function getQuantity();
} 

vg
jack

[mermshaus]

Na ja, dass ein Warenkorb eine etwas kompliziertere Angelegenheit ist als das, was wir bis dato hatten, sollte klar sein.

Mir ging es deshalb nur um formale OOP-Aspekte. Es springt in #1 ins Auge, dass User nur an CartService::add übergeben wird, weil Cart kein Objekt ist. Das verletzt sozusagen das Law of Demeter. (Wobei es auch fast müßig ist, bei einem Kontext von 20 Zeilen groß mit so was zu kommen.) User habe ich dann noch schnell mit aufgeführt wegen des Typehints auf Cart, den ich vorher als Vorteil von Cart als Objekt genannt hatte. Also, die Klassen hätten an der Stelle auch Foo und Bar heißen können. Mein „Ich hätte es so gemacht.“ ist auf den Verzicht auf den CartService und auf die Assoziationen zwischen den drei anderen erwähnten Klassen bezogen, wenn man die als gegeben annimmt.

Der letzte Satz war dann noch als „es ist dein Konzept“ gedacht. (Was ich auch noch immer so sehe, auch wenn jack88 jetzt angefangen hat, hier Ideen zu posten. Das ist grundsätzlich ein Thema, das man simpel abhandeln oder sich nahezu beliebig kompliziert machen kann. One size fits all finde ich da schwierig.)

Edit: ↓ Nicht jeder Warenkorb braucht zum Beispiel derlei Nuancen. Aber manche brauchen auch noch mehr. *schulterzuck*

[erc]

Deswegen würde ich prinzipiell von dem zweiten Ansatz abraten, da er einen nur unnötig einschränkt und sonst keine Vorteile mitbringt.

Wie du schreibst:

Das stimmt. Es kommt auch auf die Produkte an.

Bei einem Schuhe kannst du nicht einfach mal 2 Nummer größer drauf packen und das Ding braun anmalen. Da hast dus mit anderen Anforderungen zu tun als bei einer Pizza. Bei einem Schuhe hast du Variationen, bei einer Pizza Individualisierung...

[cassidy]

@all
Danke für die vielen Tipps, ich habe meinen Code nach dem Vorschlag von mermshaus umgebaut.

@jack88
Mir gefällt dein Vorschlag, aber ich finde ihn für mein Vorhaben etwas 'too much'. Der Sinn dieses Projektes ist, einen Einstieg in PHP/Symfony zu kriegen. Außerdem bin ich zwar mit MVVM vertraut, aber MVC ist noch sehr dünnes Eis für mich.
Meine Baustellen sind dementsprechend wesentlich banaler und grundlegender, weswegen ich das Backend so einfach wie möglich halten möchte.