review my linked list

[Mr_Jack]

Please reply with your comments.

#ifndef linkedlist_h
#define string_h

#include<iostream>
using namespace std;

int LLstrlen(char[]);
int LLstrlen(char str[])
{
	int i = 0;
	while(str[i] != '\0')
		i++;
	return i;
}

class node
{
private:
	int elementNum;
	node * next;
	node * prev;
	char ch;
public:
	friend class string;
	operator=(char);
	node();
	friend ostream& operator<<(ostream&, const node&);
};
node::node()
{
	next = 0;
	prev = 0;
}
node::operator=(char c)
{
	this->ch = c;
}
ostream& operator<<(ostream & os, const node & n)
{
	os << n.ch;
	return os;
}

class linkedlist
{
private:
	node * head;
	node * current;
	node * tail;
	int len;
public:
	friend ostream& operator<<(ostream&, linkedlist&);
	linkedlist();
	void clearList();
	void calcLen();
	int getLen();
	void createNext();
	void gotoNext();
	void gotoPrev();
	void gotoHead();
	void gotoTail();
	void gotoElement(int);
	void setCurCh(char);
	char getCurCh()const;
	void setCurElementNum(int);
	char * createlinkedlist();
	operator=(char[]);
	node operator[](int);
	friend void operator>>(istream&, linkedlist&);
};
node linkedlist::operator[](int e)
{
	gotoElement(e);
	return *current;
}
ostream& operator<<(ostream & os, linkedlist & ll)
{
	ll.gotoHead();
	for(int i = 0; i < ll.len; i++)
	{
		os << ll.getCurCh();
		ll.gotoNext();
	}
	return os;
}
linkedlist::operator=(char str[])
{
	clearList();
	int len = LLstrlen(str);
	for(int i = 0; i < len; i++)
	{
		setCurCh(str[i]);
		setCurElementNum(i);
		createNext();
		gotoNext();
	}
	this->len = len;
}
void operator>>(istream& is, linkedlist& ll)
{
	ll.clearList();
	char ch;
	is.get(ch);
	ll.setCurCh(ch);
	ll.setCurElementNum(0);
	int len = 0;
	int i = 1;
	while(ch != '\n')
	{
		is.get(ch);
		ll.createNext();
		ll.gotoNext();
		ll.setCurCh(ch);
		ll.setCurElementNum(i);
		len++;
		i++;
	}
	ll.len = len;
}
linkedlist::linkedlist()
{
	head = new node;
	current = head;
	len = 0;
	tail = current;
}
void linkedlist::clearList()
{
	node * temp;
	gotoTail();
	for(int i = 0; i < len-1; i++)
	{
		temp = tail->prev;
		delete current;
		current = temp;
	}
	head = current;
	tail = current;
}
void linkedlist::calcLen()
{
	if(len == 0)
	{
		while(current->next != 0)
		{
			len++;
			current = current->next;
		}
	}
}
int linkedlist::getLen()
{
	return len;
}
void linkedlist::createNext()
{
	current->next = new node;
	current->next->prev = current;
}
void linkedlist::gotoNext()
{
	current = current->next;
}
void linkedlist::gotoPrev()
{
	current = current->prev;
}
void linkedlist::gotoHead()
{
	current = head;
}
void linkedlist::gotoTail()
{
	current = tail;
}
void linkedlist::gotoElement(int n)
{
	gotoHead();
	while(current->elementNum != n)
		gotoNext();
}
void linkedlist::setCurCh(char c)
{
	current->ch = c;
}
void linkedlist::setCurElementNum(int n)
{
	current->elementNum = n;
}
char linkedlist::getCurCh()const
{
	return current->ch;
}
char * linkedlist::createlinkedlist()
{
	calcLen();
	char * str = new char[len];
	str[len] = '\0';
	gotoHead();
	for(int i = 0; i < len; i++)
	{
		str[i] = current->ch;
		gotoNext();
	}
	return str;
}
#endif

[joshdick]

Abstraction. The user of your linked list class shouldn't have to know about heads or tails or anything pointer related. A linked list class should act a lot like an array.

Also, your class would be a lot more useful if it were a template. That way, you could store whatever you wanted in it.

[ripper079]

I totaly agree with joshdick. If I were to use your linked list I really don´t care how the underlying implementation looks like.I want a linked list with certain operations (independet of what datatype im using in it) that a linked list should support. Check this recent thread for more information. Also some suggesetions
Why

• define int LLstrlen(char[]); when u got strlen?
• have a void linkedlist::calcLen()? Update linkedlist::len when you remove/add an element
• friend class string; For what purpose? maybe friend class linkedlist?

[Mr_Jack]

The user doesn't need to know anything about the inner working of the class. I guess I should have made those gotoNext, gotoPrev functions private, since they are only used internally.
The reason for making my own string len function is to see if I was able to do it effectively. calcLen is there for internal use, so I should have made it private.

Thanks for your input!

[Prelude]

Encapsulation is good, but a good container class shouldn't exactly be a black box. Let's take a look:

>friend ostream& operator<<(ostream&, linkedlist&);
Good, outputing a list with one little command is desirable.

>linkedlist();
Empty list construction, also good.

>void clearList();
This would be better named "clear". A linked list doesn't have to actually be implemented as a linked list. I've seen list containers that used binary trees as the underlying implementation. Keep your names short, easy to remember, and don't give away too much about the implementation so that it can be changed later if need be.

>void calcLen();
This is not needed. You can keep it private and have getLen call it if the length changed between calls to getLen. For example, a flag in the modification routines. If the flag is set, you assume that the length needs to be recalculated.

>int getLen();
This would be better named "size", or just "length".

>void createNext();
This can be made private and a more generic insertion routine added in its place for the client interface.

>void gotoNext();
This is good as an interface function. Though goto need not be there, you can simply use "next" and have the function return an iterator to the next item. Just as an aside, using a current iterator in your container isn't the best of ideas. It would be better to let the user create multiple iterators into the list to give them greater flexibility.

>void gotoPrev();
See above.

>void gotoHead();
Good, though "first" would be a better name.

>void gotoTail();
Good, though "last" would be a better name.

>void gotoElement(int);
Be wary of this kind of indexing operation. I wouldn't suggest including it unles you can make it efficient. Generally, this can be trivially done by the client the way you are doing it, so it really shouldn't be a part of the interface at all.

>void setCurCh(char);
>char getCurCh()const;
Use iterators to access an item, this will work, but it is difficult to do well in a generic way.

>void setCurElementNum(int);
This isn't needed anywhere, it just adds fluff and bloat. Unless you have a good reason for adding bloat, don't.

>char * createlinkedlist();
Awful naming. This suggests you are creating a linked list, which violates encapsulation and lies about what it does. What it actually does is takes the list and makes a string out of it. A better name would be "tostring".

>operator=(char[]);
Good. An itnerface usually needs an assignment operator, but I notice a distressing lack of destructor and copy constructor.

>node operator[](int);
Be very careful with indexing, using the subscript operator in particular. Clients will think instinctively that it is as efficient as array indexing, which is just not possible with linked lists. The result will be the client using your class in a very inefficient manner.

>friend void operator>>(istream&, linkedlist&);
This is okay for your list of char, but for generic lists it doesn't make much sense and would be difficult to implement properly.