Linked List program

Question

This is a simple linked list program which creates a list by appending an object at the tail. It compiles and runs perfectly.

Is the coding style, logic etc are fine? How can I improve this program? Is there anything redundant or did I miss out some important things?

#include<iostream>
#include<string>
using namespace std;
class Link_list {
private:
    string name;
    Link_list *next_node;
public:
    void add_item(Link_list *);
    void add_item();
    friend void show(Link_list *sptr)
    {
        while(sptr) {
        cout << sptr->name << endl;
        sptr = sptr->next_node;
        }
     }
};

void Link_list::add_item()
{
    cin >> name;
    next_node = NULL;
}
void Link_list::add_item(Link_list *pptr)
{
    cin >> name;
    next_node = NULL;
     pptr->next_node = this;
 }


int main()
{
    Link_list *str_ptr = NULL;
    Link_list *curr_ptr = str_ptr;
    Link_list *prev_ptr;
    char ch = 'y';

    str_ptr = new(Link_list);
    str_ptr->add_item();
    curr_ptr = str_ptr;
    do
    {
        prev_ptr = curr_ptr;
        curr_ptr = new(Link_list);
        curr_ptr->add_item(prev_ptr);
        cout <<"Do you want to add the item" << endl;
        cin >> ch;
    }while(ch != 'n');
    show(str_ptr);
  }

Answer 1

You can't remove elements from it.

There is no search feature.

All you can do is add stuff to it and then have its contents streamed to STDOUT.

It can only hold strings.

Blocking for user-input in the List class itself seems odd; usually you'd request the input in the calling scope and then pass the resultant new string to your add member function.

Answer 2

I cleaned up your code a bunch and described each change I made:

#include<iostream>
#include<string>

using namespace std;

class List {
public:
    List();
    List(List* prev);

    static void show(List* list) {
        while (list) {
            cout << list->name << endl;
            list = list->next;
        }
    }

private:
    string name;
    List* next;
};

List::List(string name)
:   name(name),
    next_node(NULL) {}

List::List(string name, List *prev) 
:   name(name),
    next_node(NULL) {
    prev->next = this;
}

int main() {
    string name;
    cin >> name;
    List* str = new List(name);
    List* curr = str;
    List* prev;
    char ch = 'y';
    do {
        prev = curr;
        cin >> name;
        curr = new List(name, prev);
        cout << "Do you want to add the item" << endl;
        cin >> ch;
    } while(ch != 'n');
    List::show(str);
}

1. Your indentation is inconsistent, which makes it hard to understand the block structure.

2. Those add_ functions should be constructors. If you new a Link_list node and then don't call one, you get a broken uninitialized node. Good style is that a class shouldn't allow itself to be in an incomplete state. Moving that add_ functionality directly into the constructors fixes that.

3. There's no need for show to use friend.

4. Either put { on the same line, or on their own, but don't mix the two. Style should be consistent or it's distracting.

5. new(Link_list) is not the normal syntax for dynamic allocation. Should be new Link_list().

6. pptr and sptr aren't helpful names. You don't really need to add _ptr all over the place either.

7. show() doesn't access any instance state, so doesn't need to be an instance method.

8. Link_list is a weird naming style. People using underscores for word separators rarely use capital letters too.

9. Reading user input directly in the list class is weird. Try to keep different responsibilities separate. List's responsibility is the data structure, not talking to stdin.

10. In C++ (as opposed to older C) you don't have to declare variables at the top of a function. Convention is to declare them as late as possible to minimize their scope.

11. Use constructor initialization lists when you can.

12. next_node is a strange name since you don't use "node" anywhere else to refer to the list.

13. I tend to put public stuff before private stuff since that what user's of your code will need to read the most.

14. You never actually de-allocate the nodes you create, but I didn't fix that here.

Answer 3

I'm just going to comment on this from a high level since others have called out other details...

Calling the object a link_list is misleading, since it isn't actually the list, but just a node in the full list. You might want to think about refactoring it so that you have an actual list, that has list_node's internal to it. Your inserts also shouldn't expose those nodes directly, but have the ability to just take the data they want to insert. There's no need to expose the behavior of the list to the user.

Answer 4

You should use std::list... But if you only wanna learn how the Linked List works, i suggest the using of templates classes and functions for make the code more generic as possible...

It isnt very difficult:

template <typename T> 

class List
{
public:
    //Example declare the "get" and return a T element
    //where T is a generic element or data type
    T get_front() const;
private:
    T data;
    List<T> *firt_Ptr;
}

In the main file:

int main()
{
    ...
    List< int > listofints;
    List< double > listofdoubles;
    ...
}

Answer 5

In main() you are managing resources. Since the program manages resources( i.e., using new operator ), it should also return resources using delete operator. So, you should start deallocating the resources from the end point of the list before program termination. Else memory leaks prevail.

Answer 6

Adding to Tomalak answer :

=> destructor

You create your linked list but how do you plan to delete it ?

=> You can only iterate in one way

When you have 1000 element and you want the last...