C++ remove from singly linked list I asked a similar question (C++ linked list) but I'd like to have this similar code reviewed. It's not the same code, just slightly different and with bugs fixed.

The code takes the head pointer of a singly linked list and an int element to find and remove from the list:

void removeFromList(node* &amp;head, int toBeRemoved){
    if (head != nullptr){
        node* temp = head;
        if (head-&gt;data == toBeRemoved){
            head = head-&gt;next;
            delete temp;
        }
        else{
            while (temp-&gt;next != nullptr &amp;&amp; temp-&gt;next-&gt;data !=toBeRemoved)
                temp = temp-&gt;next;

            if (temp-&gt;next == nullptr)
                //some code to be executed when element not found
                else{
                if (temp-&gt;next-&gt;data == toBeRemoved){
                    node* removeThis = temp-&gt;next;
                    temp-&gt;next = temp-&gt;next-&gt;next;
                    delete removeThis;
                }
            }
        }
    }
    else
        //some code to be executed when list is empty
}


I've tested it by outputting messages to the console in case the element is not found or the list is empty, and it seems to be working in every case.

Still, I'd like to know if the code can be improved (not trying to use the C++ list implementation, just to rewrite this in the best way).
Design

You are obviously not using classes (as this is a free function). This means you are exposing the implementation details of the linked list to the user. This is bad as you can not guarantee any of the invariants of the list.

Invariants:


All the nodes were allocated with new.
Does your list use sentinels? (not at the moment see below).
(Sure I could come up with more).


Personally I would wrap this up in a class where the user never sees a node object.

Code Re-Use

You have repeated chunks of code that do the same things.

void removeFromList(node* &amp;head, int toBeRemoved){
    // STUFF
        if (head-&gt;data == toBeRemoved){
            head = head-&gt;next;
            delete temp;
        }
    // STUFF
                    temp-&gt;next = temp-&gt;next-&gt;next;
                    delete removeThis;
                }


You should isolate code that does the same thing in to its own function/method. Then use a decent function name so the code becomes self documenting.

Sentinel

When writting list it is sometimes useful to have a sentinel node. This is a special node in the list that does not carry any data. But it makes manipulating the list easy as you no longer need to worry about a null value (or empty list). Thus the head no longer becomes a special case.

Example: A sentinel list will always contain one item at the head (this head item contains no data). The rest of the API should be coded with the understanding of the sentinel value. So en empty list contains only the sentinel value.

With this assumption we can simplify the code too:

void removeFromList(node* head, int toBeRemoved){
    node* temp = head;
    while (temp-&gt;next != nullptr &amp;&amp; temp-&gt;next-&gt;data !=toBeRemoved) {
        temp = temp-&gt;next;
    }
    if (temp-&gt;next == nullptr &amp;&amp; temp-&gt;next-&gt;data == toBeRemoved){
        node* removeThis = temp-&gt;next;
        temp-&gt;next = temp-&gt;next-&gt;next;
        delete removeThis;
    }
}


Use Braces

You don't consistently use braces. This is going to come back and haunt you one day. Any nested statement should have braces (even if it is a single item). Doing this one thing will save you from so many bugs.

            if (temp-&gt;next == nullptr)
                //some code to be executed when element not found
                else{
                }


Just add the braces after the if(). Also align your code better.

            if (temp-&gt;next == nullptr) {
                //some code to be executed when element not found
            }
            else {
                // Other stuff.
            }


Types and names

In C++ the most important part of the program is the type information. There is nothing more important than types. Get your types correct and you are 85% there in a C++ program.

As a result it is useful to be able to distinguish types from objects in the code. So a common convention is to use a naming scheme. A very common naming scheme is that user defined types have an initial uppercase letter while objects (variables/methods/functions) have an initial lower case letter.

// So
Node*  item;
// rather than
node*  item;


While we are taking about types. The '*' and the '&amp;' are part of the type information so they go with the type not the object. This is the inverse of how C programmers do it.

// So
Node*&amp;  head
// Rather than
node*   &amp;head


Note: Notice how the SO code block picks up the type. This is a reflection of common conventions used in C++.

Re-Design

class LinkedList
{
    struct Node
    {
        Node* next;
        virtual ~Node(){}
    };
    struct DataNode: public Node
    {
        DataNode(Node* next, int val)
            : Node(next)
            , data(val)
        {}
        int    data;
    }

    Node  sentinel;    // Notice I don't dynamically allocate this.

    public:
        ~LinkedList()
        {
            Node* next;
            for(Node* loop = sentinel.next; loop; loop = next) {
                next = loop-&gt;next;
                delete loop;
            }
        }
        LinkedList()
            : sentinel{}  // forces zero-initialization
        {}
        // Delete compiler generated methods.
        LinkedList(LinkedList const&amp;)            = delete;
        LinkedList&amp; operator=(LinkedList const&amp;) = delete;

        void add(int val)
        {
            sentinel.next = new DataNode{sentinel.next, val};
        }
        void del(int val)
        {
            for(Node* loop = &amp;sentinel; loop-&gt;next; loop=loop-&gt;next) {
                if (dynamic_cast&lt;DataNode*&gt;(loop-&gt;next)-&gt;data == val) {
                    Node* del = loop-&gt;next;
                    loop-&gt;next = del-&gt;next;
                    delete del;
                    break;
                }
            }
        }
};


To see an example of a fully implemented doubly linked list check out this answer.