0
\$\begingroup\$

Can someone tell me how I can improve this code? It works ok, but I feel that I'm using too many pointers.

#include <iostream>
using namespace std;
struct link{
    int data;
    link *next;
};
class link_list{
private:
    int boundaries = 0;
    link *tail;
    link *temp;
    link *head = NULL;
public:

    void add_node(int data){

        temp = new link;
        temp->data = data;
        temp->next = NULL;
        if (head == NULL){
            head = temp;
            boundaries++;
        }
        else{
            link *tail = head;
            while (tail->next != NULL){
                tail = tail->next;
            }
            tail->next = temp;
            boundaries++;
        }
    }
    void delete_b(){
        link *end = head;
        link *next_to_end = NULL;
        while (end->next != NULL){
            next_to_end = end;
            end = end->next;
        }
        next_to_end->next = NULL;
        end = NULL;
        boundaries--;
    }
    void add_front(int data){
        link *ptr = new link;
        ptr->next = head;
        ptr->data = data;
        head = ptr;
        boundaries++;

    }
    void delete_front(){
        link *new_head = head;
        new_head = head->next;
        head = NULL;
        head = new_head;
        boundaries--;

    }


    void add_by_position(int data, int pos){
        link *node = new link;
        link *linker = head;
        node->data = data;
        for (int i = 0; i < pos; i++){
            linker = linker->next;
        }
        node->next = linker;
        linker = head;
        for (int i = 0; i < pos - 1; i++){
            linker = linker->next;
        }
        linker->next = node;
        boundaries++;

    }

    void show(){
        link *sh = head;
        while (sh){ cout << sh->data << endl; sh = sh->next; }

    }



};


int main()
{
    link_list obj;
    obj.add_node(7);
    obj.add_node(5);

    obj.add_node(8);


    obj.show();
}
\$\endgroup\$
5
\$\begingroup\$
struct link{
    int data;
    link *next;
};

Are you sure you want this to be outside of the class? This could be in the private section in the class link_list since the outside world does not need to know of this class at all.


int boundaries = 0;

What is boundaries? It seems like this is your actual size of the list but I had to check your code to make sure what it is. Call it size or entries or something like this.


link *tail;
link *temp;
link *head = NULL;

There is no reason to have temp as a instance variable. And you do not use tail in your code, so drop them both. And use nullptr instead of NULL.


void add_node(int data){

    temp = new link;
    temp->data = data;
    temp->next = NULL;
    if (head == NULL){
        head = temp;
        boundaries++;
    }
    else{
        link *tail = head;
        while (tail->next != NULL){
            tail = tail->next;
        }
        tail->next = temp;
        boundaries++;
    }
}

Again, use nullptr. And if you had used your tail you wouldn't need to iterate over the whole list to get to the end.


void delete_b(){
    link *end = head;
    link *next_to_end = NULL;
    while (end->next != NULL){
        next_to_end = end;
        end = end->next;
    }
    next_to_end->next = NULL;
    end = NULL;
    boundaries--;
}

What's b? (Oh yeah, I think I don't need to remind you about nullptr again.)


void add_front(int data){
    link *ptr = new link;
    ptr->next = head;
    ptr->data = data;
    head = ptr;
    boundaries++;

}

add_node and add_front? What's the difference? Sure, I know what the difference is; I've seen the code. However, I don't like to look through the code to see what a function does. Call them add_front and add_back or something like this (head and tail might fit better, programmers will understand).


void delete_front(){
    link *new_head = head;
    new_head = head->next;
    head = NULL;
    head = new_head;
    boundaries--;

}

What is this playing around? That's three useless lines of code:

void delete_front(){
    link* tmp = head;
    head = head->next;
    delete tmp;
    boundaries--;
}

void add_by_position(int data, int pos){
    link *node = new link;
    link *linker = head;
    node->data = data;
    for (int i = 0; i < pos; i++){
        linker = linker->next;
    }
    node->next = linker;
    linker = head;
    for (int i = 0; i < pos - 1; i++){
        linker = linker->next;
    }
    linker->next = node;
    boundaries++;

}

by_position sounds silly. Call it add_at. And why do you reset linker and start over again? That's totally unnecessary.

void add_at(int data, int pos){
    link *node = new link;
    node->data = data;
    boundaries++;
    if(pos == 0) {
      node->next = head;
      head = node;
      return;
    }

    link *linker = head;
    for (int i = 0; i < pos - 1; i++){
        linker = linker->next;
    }
    node->next = linker->next;
    linker->next = node;
}

You should also delete the nodes in your delete/remove functions.

That's everything for now. You could've easily made a template out of this class, however, you should first fix your problems and then - if you like - ask another question with your new code.

\$\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.