4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ I'll be delighted to review your code the moment it works... coliru.stacked-crooked.com/a/72acde890732346b \$\endgroup\$ – Deduplicator Nov 22 '15 at 20:18
  • \$\begingroup\$ The code is tested and it works atleast on my machine. \$\endgroup\$ – Antithesis Nov 22 '15 at 20:18
  • \$\begingroup\$ I've added the assignment operator and modified the copy constructor, if that is what you're talking about. That aside the code works fine. \$\endgroup\$ – Antithesis Nov 22 '15 at 20:20
  • \$\begingroup\$ Clearly there's something wrong with your compiler which says that there's no member cout or endl in std. \$\endgroup\$ – Antithesis Nov 22 '15 at 20:21
  • \$\begingroup\$ @Antithesis No, I don't know how it's working for you when you haven't included the <iostream> header. \$\endgroup\$ – user2296177 Nov 22 '15 at 20:22
4
\$\begingroup\$

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]

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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