ShoppingCart and Item Classes

Question

I've built a class for an e-commerce Item, and the ShoppingCart class for checkout functions.

The thing I'm most concerned about is my use of pointers. I think I've used them properly in this situation, but I could be wrong.

Item.hpp

#ifndef Item_hpp
#define Item_hpp

#include <iostream>
using namespace std;

class Item {

private:
    string name;
    double price;
    int quantity;

public:
    void setName(string);
    void setPrice(double);
    void setQuantity(int);
    string getName();
    double getPrice();
    int getQuantity();

    // Constructors
    Item();

    Item(string, double, int);

};

#endif

Item.cpp

#include "Item.hpp"
#include <iostream>
using namespace std;

Item::Item() {
    name = "";
    price = 0.0;
    quantity = 0;
}

Item::Item(string itemName, double itemPrice, int itemQuantity) {
    setName(itemName);
    setPrice(itemPrice);
    setQuantity(itemQuantity);
}

void Item::setName(string itemName) {
    name = itemName;
}

void Item::setPrice(double itemPrice) {
    price = itemPrice;
}

void Item::setQuantity(int itemQuantity) {
    quantity = itemQuantity;
}

string Item::getName() {
    return name;
}

double Item::getPrice() {
    return price;
}

int Item::getQuantity() {
    return quantity;
}

ShoppingCart.hpp

#ifndef ShoppingCart_hpp
#define ShoppingCart_hpp

#include "Item.hpp"
#include <iostream>
using namespace std;

class ShoppingCart {

private:
    Item* itemArray[100];
    int arrayEnd;

public:
    void addItem(Item *);
    double totalPrice();

    // Constructors
    ShoppingCart();

};
#endif

ShoppingCart.cpp

#include "ShoppingCart.hpp"
#include "Item.hpp"
#include <iostream>
using namespace std;

ShoppingCart::ShoppingCart() {
    for (int i=0; i<100; i++) {
        itemArray[i] = {NULL};
    }
    arrayEnd = 0;
}

void ShoppingCart::addItem(Item *currentItem) {
    itemArray[arrayEnd] = currentItem;
    arrayEnd++;
}

double ShoppingCart::totalPrice() {
    double totalPrice;

    for (int i=0; i<100; i++) {
        if (itemArray[i] != NULL) {
            totalPrice += itemArray[i]->getPrice() * itemArray[i]->getQuantity();
        } else {
            break;
        }
    }

    return totalPrice;
}

Answer 1

• using namespace std is considered a bad practice.

• Unrestricted getters and setters are as good as making Item's members public. In fact, if there is no class invariant to maintain, do make them public.

• arrayEnd is not a particularly good name. Consider calling it items (or items_count) instead. As a side note, use it!

  for (int i = 0; i < items; i++)
      total_price += whatever;
  

  This way the shopping cart constructor reduces to

  ShoppingCart() {
      items = 0;
  }
  

• totalPrice is not initialized.

• Having pointers in the shopping card is a dubious decision. You must answer the question who owns items. In other words, who's responsibility is to destroy Item, and what happens to the cart when it is destroyed? I strongly recommend to either have the cart to keep objects, or make cart responsible for their construction and destruction.

Answer 2

While I think @vnp has the right idea, I'd go a bit further: there's no point (excuse the pun) in using pointers here.

There's also no point in creating your own dynamic array when there's a perfectly good one in the standard library (std::vector).

Your item class is much longer and clumsier to use, but ultimately provides no advantage over just:

struct item { 
    string name;
    double price;
    int quantity;
};

...and possibly having a default constructor to initialize the price/quantity to 0.

Your ShoppingCart is nearly equivalent to just a std::vector<item>, with one extra function to add up the prices of items in the cart. For that I'd probably write code something like this:

typedef std::vector<item> ShoppingCart;

double total_price(ShoppingCart const &cart) { 
    return std::accumulate(cart.begin(), cart.end(), 0.0, 
               [](double a, item const &i) { return a + i.price * i.quantity; });
}

When writing classes, keep the bottom line in mind. The intent is not to just write more code to do the same things. The real intent is to provide a higher level of abstraction that simplifies code that uses the class. In this case, that doesn't seem to be happening much (if at all). You're adding a lot of code, but in the end, most of that code is just doing what can/would happen by default if you didn't write anything. In such a case, either find something better to do, or else just don't do it.

If you wanted to think about the "something better" to do, I can see a few obvious possibilities. First would be for Item to provide some TotalPrice sort of thing that handles multiplying the unit price by the quantity to get the total price. Likewise, the ShoppingCart could compute a shipping price based on the total number of items, a weight for each, and (if you wanted to get really elaborate) codes for the source and destination for each as well. Add in a Catalog that keeps track of the number of units available so a person can't add more items to their shopping cart than there are available, and we're starting to add enough intelligence that the classes stand a good chance of really accomplishing something.