Singly-linked grocery list

For my own linked list implementation, I've decided to try something different: a linked list of groceries.

The main differences between this implementation and a standard list are:

• more data fields (name, quantity, if item has been purchased)

• two size data members (different items and total items)

• depending on quantity to be removed, an entire node may not be removed

I don't have any particular concerns, and I'd like a general review of this.

I've tested this with some test cases on Ideone.

GroceryList.hpp

#ifndef GROCERY_LIST_HPP
#define GROCERY_LIST_HPP

#include 
#include 

class LinkedGroceryList
{
private:
    struct Grocery
    {
        std::string name;
        std::size_t quantity;
        bool purchased;
        Grocery* next;
    };

    Grocery* head;
    std::size_t differentGroceries;
    std::size_t totalGroceries;

public:
    LinkedGroceryList();
    ~LinkedGroceryList();
    void addGrocery(std::string const&, std::size_t);
    void removeGrocery(std::string const&, unsigned int);
    bool findGrocery(std::string const&) const;
    void markAsPurchased(std::string const&);
    void displayGroceries() const;
    void clearGroceries();
    std::size_t different() const { return differentGroceries; }
    std::size_t total() const { return totalGroceries; }
    bool empty() const { return differentGroceries == 0; }
};

#endif

GroceryList.cpp

```
#include "GroceryList.hpp"
#include

LinkedGroceryList::LinkedGroceryList()
: head(nullptr)
, differentGroceries(0)
, totalGroceries(0)
{}

LinkedGroceryList::~LinkedGroceryList()
{
clearGroceries();
}

void LinkedGroceryList::addGrocery(std::string const& name, std::size_t quantity)
{
Grocery* newGrocery = new Grocery;

newGrocery->name = name;
newGrocery->quantity = quantity;
newGrocery->purchased = false;

newGrocery->next = nullptr;

if (!head)
{
head = newGrocery;
}
else
{
Grocery* groceryPtr = head;

You need to consider the Rule of Three.

That is: the default copy and assignment implementations will not handle the head as you'd like. You need to define them correctly, or hide them (or remove them in C++ 11).

For example:

class LinkedGroceryList
{
private:
    LinkedGroceryList(const LinkedGroceryList& other);
public:
// etc...
};

I agreed with @Corbin as to the names for the sizes, but would consider using size() to hold the length of the list, i.e. differentGroceries to ape the standard library. Likewise clear rather than clearGroceries, as the element type is implicit in the container name.

Similarly (and some would disagree), I prefer findGrocery to be findGroceryByName. That should not just return a bool, but ideally an iterator to the element with the grocery. Alternatively, rename to containsGroceryWithName or containsGrocery.

Also consider some checking of invariants. For example, differentGroceries == 0 head == nullptr.

In the method markAsPurchased, you should break out of the loop on a match. I'd also return bool indicating success in finding a match.

Why does add take a size_t and remove an unsigned?

In the remove method, for the head case:
1. You reassign the head without considering whether or not all it items are removed.
2. You should check to having a negative number or items which becomes an underflow.
3. The head and main case have common code that should be in a common method.

In the add method, you need to check to see if your list already contains an item with that name. Actually there is a bigger problem here. If you merge items when added with the same name, you'll lose track of what is purchased. If you do not, you'll have much more complexity is remove. And what does remove mean for items that may be purchased or not?

I think this means that purchased is not a bool, but a count of the number of items with the given key that have been purchased. The markAsPurchased method could, but does not necessarily need to, be updated to specify the number of items purchased.

StackExchange Code Review Q#45992, answer score: 7