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

• Deduplicator: I'll be delighted to review your code the moment it works... coliru.stacked-crooked.com/a/72acde890732346b
• Antithesis: The code is tested and it works atleast on my machine.
• Antithesis: 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.
• Antithesis: Clearly there's something wrong with your compiler which says that there's no member cout or endl in std.
• user2296177: @Antithesis No, I don't know how it's working for you when you haven't included the <iostream> header.

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]