Singly Linked List in C++

Question

I've written a singly linked list in C++. Would appreciate reviews on code efficiency and best practices.

#ifndef SLIST_HPP
#define SLIST_HPP

#include <stdexcept>
#include <iostream>

template <typename T> struct node{
    node(T data):data(data),next(nullptr){}
    T data;
    node<T> * next;
};

template< typename T> class slist{
    node<T>* head;
    int size;
public:
    slist(node<T>* head):head(head), size(0){}

    slist(const slist<T>& rhs){

     node<T>* temp = rhs.getHead();

    node<T>* current = new node<T>(temp->data);
    while(temp != nullptr){

        current->next = new node<T>(temp->next->data);
        temp = temp->next;
        current = current->next;
    }
}
    ~slist(){
        if(head == nullptr) return;

        while(head != nullptr){
            node<T>* current = head;
            head = head->next;
            delete(current);
        }
    }
    slist& operator= (const slist& rhs){

    node<T>* temp = rhs.getHead();
    try{
        if(temp == nullptr) throw std::runtime_error("Can't assign null");
    }catch(std::runtime_error& e){
        std::cout<<e.what()<<std::endl;
    }

    node<T>* current = head;
    while(temp != nullptr){

        temp = temp->next;
        current = current->next;
        current->next = new node<T>(temp->next->data);
    }


}
    node<T>* getHead()const {
        return head;
    }

    void add(node<T>* p, T item){
        if(p == nullptr) return;
        node<T>* current = new node<T>(item);
        node<T>* temp = p->next;
        p->next = current;
        current->next = temp;
        size++;

    }
    void insertFront(T item){
        node<T>* p = new node<T>(item);
        if(head == nullptr){
            p = head;
            size++;
            return;
        }
        p->next = head;
        head = p;
        size++;
    }

    void insertBack(T item){
        node<T>* p = new node<T>(item);
        node<T>* current = head;
        while(current->next != nullptr){
            current = current->next;
        }
        current->next = p;
        size++;
    }

    void remove(T item){
        bool check = false;

        node<T>* current = head;
        try {
            while(current != nullptr){
                if(current->data == item) check = true;
                current = current->next;
            }
            if(!check){
                throw std::runtime_error("Item not in list");

            }
        }catch(std::runtime_error& e){
            std::cout<<e.what()<<std::endl;
            exit(-1);
        }

        current = head;
        while(current != nullptr){
           if(current->next->data == item){
               node<T>* temp = current->next;
               current->next = current->next->next;
               delete(temp);
               break;
           }
            current = current->next;
        }
        size--;

    }
    int getSize () const {
        return size;
    }

    void printList(){
        node<T>* current = head;
        while(current != nullptr){
            if(current->next != nullptr){
                std::cout<<current->data<<"->";
            }else{
                std::cout<<current->data<<std::endl;
            }
            current = current->next;
        }
    }
};
#endif //SLIST_HPP

Answer 1

You're reimplementing std::forward_list, so a good place to start would be the API documentation for forward_list. That'll show you a lot of deficiencies in your current approach:

• Naming. For example, getSize() should be size(), so that you can write generic algorithms that work with your class in addition to std::list, std::vector, etc (all of which have a size() method).

• Encapsulation. You provide a public member function getHead() that returns a raw pointer to an internal node. What do you expect the user to be able to do with a node<T>*?

• Composability. Your remove member function causes an exit(-1) on failure. It would be much much better to have remove throw an exception, and then allow the caller to catch and handle the exception — possibly with an exit, but possibly in some other way.

• Support common C++ idioms. Your list class doesn't provide begin() and end(), so it's not iterable; e.g. I can't say for (auto&& element : myList) { std::cout << element; }. If you provide iteration, then I can write this loop, so I won't need your printList member function anymore.

Finally, one more naming nit: As a member function, printList could just be named print. myList.print() is slightly better (shorter and less repetitive) than myList.printList(). And again it enables generic programming:

// You could overload this printTwice function for each container type:

void printTwice(const List& l)
{
    l.printList();
    l.printList();
}

void printTwice(const Vector& l)
{
    l.printVector();
    l.printVector();
}

// ^ But yuck. Prefer genericity:

template<class Container>
void printTwice(const Container& c)
{
    c.print();
    c.print();
}

// ^ In fact, prefer to use non-member functions for anything that
// doesn't require special access to the innards of the class:

template<class Container>
void printTwice(const Container& c)
{
    print(c);
    print(c);
}

// ^ In fact, prefer to spell the verb "print" in the standard way[*]:

template<class Container>
void printTwice(const Container& c)
{
    std::cout << c << std::endl;
    std::cout << c << std::endl;
}

[* – unless you're really concerned about efficiency, in which case you should probably stay away from iostreams; but at that point you're not doing generic programming anymore anyway]