Simple Linked List that holds an int

Question

I am inexperienced in C++. I was able to get my LinkedList to be fully functional. I was hoping to get some feedback as to whether or not I followed bad practices and if there are any risks associated with my code.

LinkedList.h

#pragma once

#include <string>

#include "Node.h"

class LinkedList
{
private:
    Node *head;
    int count;
public:
    LinkedList();
    ~LinkedList();
    int get_count();
    void AddNode(int);
    std::string GetList();
    bool Contains(int);
    void Remove(int);
    int GetValue(int);
};

LinkedList.cpp

#include <iostream>

#include "LinkedList.h"

LinkedList::LinkedList()
{
    head = new Node(0);
}

LinkedList::~LinkedList()
{
}

int LinkedList::get_count(){
    return count;
}

void LinkedList::AddNode(int _value){
    Node *node = new Node(_value);
    Node *previousNode = head;
    while (previousNode->next_node && previousNode->next_node->get_value() <= node->get_value()){
        previousNode = previousNode->next_node;
    }
    if (previousNode->next_node){
        node->next_node = previousNode->next_node;
    }
    previousNode->next_node = node;
    count++;
}

std::string LinkedList::GetList(){
    std::string list;
    Node *currentNode = head;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        list += std::to_string(currentNode->get_value()) + " ";
    }
    return list;
}

bool LinkedList::Contains(int number){
    Node *currentNode = head;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        if (currentNode->get_value() == number){
            return true;
        }
        if (currentNode->get_value() > number){
            return false;
        }
    }
    return false;
}

void LinkedList::Remove(int number){
    Node *previousNode = head;
    Node *currentNode = head;
    while (currentNode->next_node){
        previousNode = currentNode;
        currentNode = currentNode->next_node;
        if (currentNode->get_value() == number){
            previousNode->next_node = currentNode->next_node;
            delete currentNode;
            count--;
            return;
        }
    }
}

int LinkedList::GetValue(int index){
    Node *currentNode = head;
    int currentIndex = -1;
    while (currentNode->next_node){
        currentNode = currentNode->next_node;
        currentIndex++;
        if (index == currentIndex){
            return currentNode->get_value();
        }
    }
    return -1;
}

Node.h

#pragma once

class Node
{
private:
    int value;
public:
    Node(int);
    ~Node();
    Node *next_node;
    int get_value();
    void set_value(int);
};

Node.cpp

#include "Node.h"

Node::Node(int _value)
{
    value = _value;
}

Node::~Node()
{
}

int Node::get_value(){
    return value;
}
void Node::set_value(int _value){
    value = _value;
}

Answer 1

• Instead of having Node use getters and setters, just make it a struct declared within the private section of LinkedList. It won't need its own files, either.

• All but one of your functions are in PascalCase. Whether you choose that or camelCase, choose just one and maintain that. This should be kept consistent.

• When creating a new linked list, the head should first point to NULL (or nullptr in C++11) to indicate an empty list. You're instead having the head point to a new node, so the list isn't really considered empty at the start.

• You have an empty destructor, even though you allocate memory with new. In order to prevent memory leaks, the destructor should use delete on each allocated Node. This is typically done with a loop through the list.

• Member functions that don't modify data members should be const. This also prevents any accidental modifications to data members within these functions.

  An example with your get_count() function:

  int LinkedList::get_count() const {
      return count;
  }
  

Answer 2

In addition to the excellent suggestions by @Jamal...

Physical Layout of class

The public section of a class should appear first, followed by the protected section, and private section should appear last. The reason for that is that to the users of the class, the public section of a class is its most visible interface, the protected section is the next visible interface, and the private section is the least visible interface.

class LinkedList
{
public:
    LinkedList();

    ...

private:
    Node *head;
    int count;
};

Logic error in LinkedList::Remove

The posted code does not take into account the possibility that the given input corresponds to the head of the list.

void LinkedList::Remove(int number){
    Node *previousNode = head;
    Node *currentNode = head;

    // You need this.
    if ( head->get_value() == number )
    {
        Node* temp = head->next_node;
        delete head;
        head = temp;
        return;
    }

    while (currentNode->next_node){
        previousNode = currentNode;
        currentNode = currentNode->next_node;
        if (currentNode->get_value() == number){
            previousNode->next_node = currentNode->next_node;
            delete currentNode;
            count--;
            return;
        }
    }
}

Answer 3

The most important points have already been said. But here are some minor things:

Portability

#pragma once is actually non standard, thus you should consider using usual include guards if you want to support ExoticCompilerFoo.

Trailing whitespace

Your GetList function puts an extra whitespace at the end of the list, though you might already know this.

Utility function

Your GetList function could actually be implemented using only the public interface of your class. It could also be used with other classes sharing that public interface. Thus you could actually write it as a generic freestanding function.

This is, for example, what the standard library does with its algorithms.