1
\$\begingroup\$

Edit

How would I implement a class dedicated to returning html strings for a list of products/cart items?

  • Written as part of an interview assignment.
  • Simply this is a shopping website with some items a user may add or remove from their shopping cart.
  • This function is responsible for returning the html to display for the list of cart items.
  • Currently there is no dedicated class for returning html strings.
  • I feel I would be moving most of this code into another class, defeating the point.

Code:

function listItems()
{
    $formatter = new Formatter();
    $html = "";
    $id = 0;
    foreach ($_SESSION["Cart"] as $item) {
        if ($item["quantity"] > 0) {
            $html = $html .
                '<pre>
                    <div class="name">'.$this>getName($item) .'</div><div class="price">Price: $' . $formatter->formatPrice($item) . '</div>
                    <div class="quantity">Quantity: ' . $item["quantity"] . '</div>
                    <div class="total">Total: $' . $this->getTotal($item) . '</div>
                    <form class="removeForm" method="POST">
                        <input class="removeInput" type="hidden" name="removeId" value=' . $id . ' />
                        <input class="removeInput"  type="submit" name="removeButton" value="Remove from Cart" />
                    </form>
                </pre>';
        }
        $id++;
    }

    $html = $html .
        '<pre>
            <div class="overallTotal">Overall Total: $' . $this->getOverallTotal() . '</div>
        </pre>';

    return $html;
}

Formatter class method returns a number with 2 decimal point format.

Full Repository here if you want more to critique:

\$\endgroup\$
  • 1
    \$\begingroup\$ The down-vote may be triggered by introducing the code How would I implement, making it look not implemented yet - down-voters, please comment! OTOH, the post does not show the implementation of such a class: if you got one, include it. (If not, I think the question off-topic.) \$\endgroup\$ – greybeard Feb 14 at 9:02
  • \$\begingroup\$ I didn't mention anything about html output earlier and was looking for general advice to improve this function. So to specify I added a desire to have an output class. I don't really know what I want other than to "be better" which is too general. I only picked this function because I thought it could be improved, but I don't have an argument as to why I think that. \$\endgroup\$ – Nathaniel Feb 14 at 9:07
  • 1
    \$\begingroup\$ I [presented] this function because I thought it could be improved, but I don't have an argument as to why I think that one of the "use cases" for CodeReview@SE. Just stay on topic and don't ask in a way to avoid. \$\endgroup\$ – greybeard Feb 14 at 9:12
  • \$\begingroup\$ Sorry, I don't understand one of the "use cases" for CodeReview@SE. Are you saying your quote of me doesn't follow the use case? \$\endgroup\$ – Nathaniel Feb 14 at 9:26
  • \$\begingroup\$ (I left out I think that to be. But: you urgently need to remove parts about code not yet written. This question may be closed - not meaning go away, but this should not be answered unless improved.) \$\endgroup\$ – greybeard Feb 14 at 9:37
1
\$\begingroup\$

You might want to decompose it into smaller pieces that will be easier to follow and eventually modify.

You could also learn a bit about dependency injection. Creating new instances all around anytime you need them is not very good.

Here is a very simple rewrite of your code and a lot of pieces are missing but I hope it gives some general guideline.

It uses PHP 7.4 features, if you can't use those, of course ignore all the typehints, etc..

Also it might make sense to depend on interfaces instead of classes directly, but didn't want to go this far as I think it could be too much for you at one go.


Dont be shy to define structures.

class Product
{
  public int $id;
  public string $name;
  public float $price;

  public function __construct(int $id, string $name, float $price)
  {
    $this->id =  $id;
    $this->name = $name;
    $this->price = $price;
  }
}

Create product repository

class ProductsRepository
{
    /**
     * @param array<int> $ids
     * @return array<int, Product>
     */ 
    function getProducts(array $ids): array
    {
        // get it from somewhere
    }
}

Structure for item in cart holds a bit more.

class CartItem
{
  public int $productId;
  public string $name;
  public float $unitPrice;
  public int $quantity;
  public function __construct(int $productId, string $name, float $unitPrice, int $quantity = 1)
  {
    $this->productId = $productId;
    $this->name = $name;
    $this->unitPrice = $unitPrice;
    $this->quantity = $quantity;
  }
  public function getTotalPrice(): float
  {
    return $this->unitPrice * $this->quantity;
  }
}

Cart holds many items, allows to iterate them, and add them and it knows the product repository to allow it doing so.

class Cart
{
  private ProductsRepository $products;
  /** @var array<CartItem> */
  private array $items;
  public function __construct(ProductsRepository $products, array $items = [])
  {
    $this->products = $products;
    $this->items = $items;
  }
  public function getItems(): array
  {
    return $this->items;
  }

  public function getTotalPrice(): int
  {
    // count it here, or have it in property and make sure it is updated while adding items.
  }

  public function add(int $productId, int $quantity = 1)
  {
    if (isset($this->items[$productId])) {
      // already in cart => increase quatity
    } else {
      $product = $this->products->getProduct($productId);
      //handle product not exists, not on stock, etc.
      $this->items[$productId] = new CartItem($product->id, $product->name, $product->price, $quantity);
    }
  }
}

We will store cart in session, this will take care of it.

class CartSessionStorage
{
  // we need it for the Cart instances
  // and maybe we wont store product names in session but load them from products repo?
  private ProductsRepository $products;

  public function loadCart(): Cart
  {
    // load from session
  }
  public function saveCart(Cart $cart): void
  {
    // save to session
  }
}

Renderer knows the html structure and some localization/formatting rules.

class CartRenderer
{
  private Formatter $formatter;
  public function __construct(Formatter $formatter)
  {
    $this->formatter = $formatter;
  }
  public function renderCart(Cart $cart): string
  {
    $html = '';
    // your rendering here;
    foreach ($cart->getItems() as $item) {
    //you can use heredoc, but add some html escaping, nor heredoc nor your code handle it
    $html .= 
      <<<HTML
      <div>write html here<div>and dont mind if u use <span>{$item->getTotalPrice()}</span>
      HTML;
    }
    return $html;
  }
}

Put it all together.

$products = new ProductsRepository();
$storage = new SessionCartStorage(products);
$formatter = new Formatter();
$renderer = new CartRenderer($formatter);

And there you go.

$cart = $storage->loadCart();

// form submission?
$cart->add($productId, $quantity);
$storage->saveCart($cart);

// render it
echo $renderer->renderCart($cart);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the in-depth code review. I found the dependency injection design pattern the most interest and useful point. I've read up on it and think I understand the gist of it. This really helped a lot, and I will definitely be referring to this review for a while. \$\endgroup\$ – Nathaniel Feb 24 at 13:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.