Archived

This topic is now archived and is closed to further replies.

will some one please review my source

This topic is 5536 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

this is my source code for a single linked list. I think I''m doing this right, but I would like some feed back from you people.
  
#include <iostream.h>

struct node
{
	int	id;
	node *next;
};
node *head = NULL;
node *curr;

void add_head( int id )
{
	curr = new node;

	curr->next = head;
	head = curr;
	head->id = id;
}

int main()
{
	int i, nodes;

	cout << "\nenter node count::";	
	cin >> nodes;
	
	for( i = -1; i < nodes; ++i )
	{
		add_head( i );
	}

	cout << "\nlist dump..";
	curr = head;

	while( curr->next != NULL )
	{
		cout << "\ncurr->id = " << curr->id;
		curr = curr->next;
	}

	cout << "\n\n";
	return( 0 );
}
  
Once I get linked list down, I''ll be set. So any help and advice (if its''t mean) would be nice.

Share this post


Link to post
Share on other sites
1) What is "iostream.h" doing? No code written in the year 2002 should mention it.
2) Read this to save me from having to explain why your list''s interface bites. In simple terms, the users of your list should not have to concern themselves with managing the nodes of your list.

Share this post


Link to post
Share on other sites
You got the basic functionality of the linked list correctly. However :

#include <iostream.h>
That''s very bad, use <iostream> instead. Follow it with using namespace std; if you want to write cin, cout, endl and so on instead of std::cin, std::cout, std::endl...
(iostream.h is deprecated and should not be used in new code. It is only kept for compatibility with old pre-standard code).

void add_head( int id )
It works, but you should have a reference to head as a parameter to let you have multiple linked lists. And declare curr as a local variable, since the rest of the program shouldn''t know about it.

int main()
Idem in main(), curr should be a local variable.

You''re never deleting your list, nor providing a way to do it, that''s a memory leak, that''s very bad.


And no, I''m not being mean


Documents [ GDNet | MSDN | STL | OpenGL | Formats | RTFM | Asking Smart Questions ]
C++ Stuff [ MinGW | Loki | SDL | Boost. | STLport | FLTK | ACCU Recommended Books ]

Share this post


Link to post
Share on other sites
Eh, I was just trying to get a working linked list. I don''t care about global and local issues and any of that other stuff. I''m just trying to actually implement a linked list. I''m just playing with some code. It dosn''t need to be all perfect.
Any ways, its a start on a linked list. I want to know if its working correctly or not. I have a user enter a value for the program. Its suppose to add head node and travers throught the list. This program is a test to see if I got it right. Thats why theres a input for the amount of nodes. I want to control that amount. I''m just starting on linked list again. Last time I didn''t even get half this far, so I think I''m doin pretty good.

Share this post


Link to post
Share on other sites
Well, as I said, the basic functionality of your list works, we''re pointing at how it could be be better.

Honestly, until you start dealing with arbitrary insertion (not just at the beginning), and removal of nodes, there is just no way we can tell you wether it is good or bad... there is not enough to base a judgement on.



Documents [ GDNet | MSDN | STL | OpenGL | Formats | RTFM | Asking Smart Questions ]
C++ Stuff [ MinGW | Loki | SDL | Boost. | STLport | FLTK | ACCU Recommended Books ]

Share this post


Link to post
Share on other sites
Now try making a simple user interface for your linked list with these components.

User Interface:
------------------
Create Linked List

Add Node
Delete Node
Move to Head of Link List
Move to Next node

Destroy List

Good Luck!!

Share this post


Link to post
Share on other sites
quote:
Original post by sakky
Eh, I was just trying to get a working linked list. I don''t care about global and local issues and any of that other stuff. I''m just trying to actually implement a linked list. I''m just playing with some code. It dosn''t need to be all perfect.

If you''re going to get all defensive and uptight when people offer the criticism that you asked for, why bother asking for it in the first place.

But, fine.

Your code is simply wonderful. Good job. Long may you continue to produce such high quality programming.

quote:
Any ways, its a start on a linked list. I want to know if its working correctly or not.

No, it isn''t, because it''s making the user deal with nodes. I don''t want to deal with nodes! I have enough to worry about with taking care of my own objects. The list''s objects should not be my concern.

quote:
I have a user enter a value for the program. Its suppose to add head node and travers throught the list. This program is a test to see if I got it right. Thats why theres a input for the amount of nodes. I want to control that amount. I''m just starting on linked list again. Last time I didn''t even get half this far, so I think I''m doin pretty good.

Read the article I linked. Certain design decisions -- in particular, the use of sentinel nodes -- will make the implementation of the list easier than would be the case with the kind of list you''re writing. Yes, I know it''s another idea for you to think about, but at the end of the day, I think it''ll be worth it. When you have sentinel nodes, certain operations become magically easier, as you no longer have to special-case anyway.

Share this post


Link to post
Share on other sites