Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
#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);
  }

This is a simple linked list program which creates a list by appending an object at the tail. It compiles and runs perfectly. If you guys can look at it and comment whether 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?

share|improve this question
5  
This would be perfect for codereview.SE if someone could migrate it over. – Mark Loeser Mar 2 '11 at 19:49
why not just use std::list? – AJG85 Mar 2 '11 at 19:53
4  
@AJG85: Education? – Tomalak Geret'kal Mar 2 '11 at 19:57
1  
The option to migrate to codereview isn't available yet. Must be because it's still in beta. – Emile Cormier Mar 2 '11 at 19:58
Questions have been migrated over before. It should be accessible to moderators only. – Mark Loeser Mar 2 '11 at 19:59
show 1 more comment

migrated from stackoverflow.com Mar 2 '11 at 20:31

This question came from our site for professional and enthusiast programmers.

7 Answers

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.

share|improve this answer
for your last comment if i want to redesign class then in your case i have to change both the class and main. but here the way i have written i need to change only the class main remains untouched – Anonymous Mar 2 '11 at 20:13
1  
@user634615: Usually you design the class then leave it, which is what my way would give you. You expect to change things in your calling function anyway. – Tomalak Geret'kal Mar 2 '11 at 20:17

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;
    ...
}
share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

Use std::list.

share|improve this answer
1  
im not going to use it in anywhere. i just wanted to know wether something is redundant or im missing smthng in the prog that's is imp or make code more robust. things like that. Thanks for comment anyway – Anonymous Mar 2 '11 at 19:58

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

share|improve this answer
1  
That's not the problem of the code, it's the constraint of data structure – Dyppl Mar 3 '11 at 5:15

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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