1
\$\begingroup\$

My professor required we make a LinkedList class with a bool add(in newEntry) method.

Two things I'm questioning my choices on is the variable name temporary and instances of how to return False if it isn't successfully added. Or is simply not returning True is going to return something False?

Is there a better convention than calling the new Node temporary?

LinkedList.h

#pragma once

struct Node {
   int data;
   Node* next;
};

class LinkedList {
protected:
   Node* m_head;
   int currentSize;

public:
   // constructor
   LinkedList();

   // destructor

   //getters

   // setters
   /** */
   bool add(int newEntry);

};

LinkedList.cpp

/**Creates a new Node (dynamically allocated) with data = newEntry, adds it to the front of the List, and increases currentSize by 1. Returns true if the new Node with newEntry was added successfully */
bool LinkedList::add(int newEntry)
{
   Node* temporary;
   temporary = new Node;
   temporary->data = newEntry;
   temporary->next = nullptr;

   if (m_head == nullptr) {
      m_head = temporary;
   }
   else {
      temporary->next = m_head;
      m_head = temporary;
   }
   this->currentSize++;
   
   return true;
}
\$\endgroup\$
4
  • \$\begingroup\$ Where is the rest of the code to provide context? \$\endgroup\$
    – pacmaninbw
    Oct 18, 2022 at 19:13
  • \$\begingroup\$ What else should be included? I was mostly interested in the implementation. Do you want to see the .h as well? \$\endgroup\$
    – Hofbr
    Oct 18, 2022 at 19:26
  • \$\begingroup\$ We would need the LinkedList and Node class definitions/implementations, so we can provide a more in-depth review than a single function. Knowing how you constructed and wrote those classes is a huge help to reviewers. \$\endgroup\$
    – Linny
    Oct 18, 2022 at 19:28
  • \$\begingroup\$ Ok no problem, I've added the class and struct you requested. \$\endgroup\$
    – Hofbr
    Oct 18, 2022 at 19:32

2 Answers 2

2
\$\begingroup\$

The linked list class definitely needs a destructor, otherwise we leak memory.

And it definitely needs a copy-constructor and assignment operator, to avoid nodes belonging to two or more lists. They can be declared = delete, but do need to exist.

The protected members are a concern. If we intend to subclass from LinkedList then we really need a virtual destructor. However, I think in this case, it's better to make the members private (the default for class types).

Users shouldn't need to know about the type Node, so it can be a class internal to LinkedList.

We should initialise m_head and currentSize in the class; then the constructor can be = default.

It doesn't make sense for the size to be negative, so use an unsigned type for that. std::size_t is the usual choice, as it should be able to represent any number of elements that can be created in our address-space.


There doesn't seem to be much point returning a bool from the add() member function, since we never return a false value (failures are reported using exceptions).

Get into the habit of using preincrement (++x) in preference to postincrement (x++) when the value doesn't matter. For many types, the former is more efficient (can save copying objects). There's no need to explicitly write this-> when accessing member variables.

If we look at your if/else, we see that they share the last statement:

   if (m_head == nullptr) {
      m_head = temporary;
   }
   else {
      temporary->next = m_head;
      m_head = temporary;
   }

So we can move m_head = temporary to the common code path after that:

   if (m_head == nullptr) {
   }
   else {
      temporary->next = m_head;
   }
   m_head = temporary;

Then observe that prior to this, we set temporary->next to null, but we only need to do this for the first branch above, since the else branch immediately overwrites:

   if (m_head == nullptr) {
      temporary->next = nullptr;
   }
   else {
      temporary->next = m_head;
   }
   m_head = temporary;

Now we see that if m_head is null, then setting next to m_head will set it to null:

   if (m_head == nullptr) {
      temporary->next = m_head;
   }
   else {
      temporary->next = m_head;
   }
   m_head = temporary;

Which we can then write as

   temporary->next = m_head;
   m_head = temporary;

So the whole function now becomes

void LinkedList::add(int newEntry)
{
   Node* temporary = new Node;
   temporary->data = newEntry;
   temporary->next = m_head;
   m_head = temporary;
   ++currentSize;
}

And we can use aggregate initialization to set those members in the new Node object:

void LinkedList::add(int newEntry)
{
   m_head = new Node{newEntry, m_head};
   ++currentSize;
}

This neatly solves the problem you asked about naming the new node - now it doesn't even have a name!


Updated code

#include <cstddef>

class LinkedList
{
    struct Node
    {
        int data;
        Node* next;
    };

    Node* m_head = nullptr;
    std::size_t currentSize = 0;

public:
    LinkedList() = default;
    LinkedList(const LinkedList&) = delete;
    void operator=(const LinkedList&) = delete;
    ~LinkedList() noexcept;

    /** prepend the new value to the head of the list */
    void add(int newEntry);

};
LinkedList::~LinkedList()
{
    Node *current = m_head;
    while (current) {
        Node *next = current->next;
        delete current;
        current = next;
    }
}

void LinkedList::add(int newEntry)
{
    m_head = new Node{newEntry, m_head};
    ++currentSize;
}
\$\endgroup\$
2
  • \$\begingroup\$ Ooo I love that and I learned a lot. Thank you Toby. \$\endgroup\$
    – Hofbr
    Oct 19, 2022 at 18:34
  • \$\begingroup\$ Thanks for the typo fix @Deduplicator - I fixed it in one place but missed the other! \$\endgroup\$ Oct 21, 2022 at 8:54
1
\$\begingroup\$

Personally I would go for newElement, but that's mainly because I can't spell "temporary". If you added a constructor to Node, you could create the new value more elegantly:

Node (const int nData = 0, const Node* pNext = nullptr)
  : data(nData), next(pNext)
{}


m_head = new Node (newEntry, m_head);
if (m_head) {
   this->currentSize++;
   return true;
}
return false;
    

The chances of ever returning false are incredibly slim, and if it does happen, then you have more trouble than an error message will help with.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ m_head cannot be false after we've assigned the new Node pointer to it. (Perhaps you're thinking of std::malloc() which reports failure in a different way to new?) \$\endgroup\$ Oct 19, 2022 at 15:08
  • \$\begingroup\$ You don't need to write a constructor for Node like that - aggregate initialisation will work fine unless you're stuck with an ancient standards version (i.e. < C++11). Which you shouldn't be, especially as a learner. \$\endgroup\$ Oct 19, 2022 at 15:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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