Is this a missusing of the friend keyword ?

I know many uses of friend keyword usually are a sign of bad design, i've implemented a simple Doubly Linked List Class, and a LinkedListIterator to use with the LinkedList class.

For that i declared each one of them "friends" of the other.
Is there a "cleaner" way to implement an iterator ?

Code:

struct Node
{
	Node(Entry val)
	{
		next = NULL;
		previous = NULL;
		this->val = val;
	}

	Node()
	{
		next = NULL;
		previous = NULL;
	}

	Node *next;
	Node *previous;

	Entry val;
};



//!  LinkedList
/*!
Basic implementation of a linked list
*/
class LinkedList
{
public:
	int i;
	friend class LinkedListIterator;
	
	// constructors
	LinkedList(void);
	~LinkedList(void);
	
	// metods
	void Append(Entry val);
	void Prepend(Entry val);
	void RemoveHead();	
	void RemoveTail();
	void RemoveNode(LinkedListIterator *it);
	Entry Head() 
	{
		assert(m_head);

		return m_head->val; 
	}

	
	private:
	Node *m_head;	// head of the linked list
	Node *m_tail;	// tail fo the linked list

	LinkedList(const LinkedList &){};	// avoid user trying to pass by value

};

class LinkedListIterator
{
public:
	friend class LinkedList;

	// constructors
	LinkedListIterator(const LinkedList *list)
	{
		m_list = list;
		m_current = list->m_head;
	}

	~LinkedListIterator()
	{
	}

	// methods
	void Next()
	{
		m_current = m_current->next;
	}

	void Start()
	{
		m_current = m_list->m_head;
	}

	bool IsValid()
	{
		return m_current != NULL;
	}

	Entry * GetValue()
	{
		return &m_current->val;
	}
private:
	const LinkedList *m_list;	// the list we're iterating
	Node *m_current;	// current node we're in

};

I don't think that is particularly bad use of friend.

--
Mats

An iterator should be thought of as a part of the class it iterates over. There is absolutely NOTHING wrong with allowing the iterator access to the internals of the class. This is the whole point of iterators -- to insulate you from the implementation details of the container. Somewhere, some piece of code has to understand the internals. This piece of code is the iterator.

Using friend actually enhances encapsulation, it doesn't hurt it. Without friend, you might be forced to make certain members public in order to accomplish certain things. That HURTS encapsulation. Of course, it's easy to use friend to allow yourself access to a private member when this could have been accomplished another way, but this is not one of those cases.

Perfect use of the friend keyword in the list class. But the iterator and node classes should nonetheless be private inner classes of the list class. Also, there is no need for the friending of the list in the iterator.

Thanks for the feedback guys

Originally Posted by CornedBee

Perfect use of the friend keyword in the list class. But the iterator and node classes should nonetheless be private inner classes of the list class. Also, there is no need for the friending of the list in the iterator.

i need friending on the list iterator because the list has a method to acept a iterator and remove the node it currently points, followed your advice and come up with

Code:

class LinkedList
{
public:
	struct Node
	{
	public:
		Node(Entry val);

		Node();

		Node *next;
		Node *previous;

		Entry val;
	};

	friend class Iterator;


	class Iterator
	{
	public:
		friend class LinkedList;

		// constructors
		Iterator(const LinkedList *list);
		~Iterator();
		
		// methods
		void Next();
		void Start();
		bool IsValid();
		Entry * GetValue();
	private:
		const LinkedList *m_list;		// the list we're iterating
		LinkedList::Node *m_current;	// current node we're in

	};


	// constructors
	LinkedList(void);
	~LinkedList(void);

	// metods
	void Append(const Entry val);
	void Prepend(const Entry val);
	void RemoveHead();	
	void RemoveTail();
	void RemoveNode(Iterator *it);
	Entry Head();

private:

	Node *m_head;	// head of the linked list
	Node *m_tail;	// tail fo the linked list

	LinkedList(const LinkedList &){};	// enforce that the user will not pass by value a linked list

};

In fact now the syntax of using an iterator in code is

LinkedList::Iterator it(....);
which is similar to the stl syntax (off course being miles and miles away from it ^^)

The Node inner class should be private.

You can remove one friendship if you give the iterators to users like standard containers do - through a begin() and end() method. In this case you could remove the list pointer from the iterator and just keep the node pointer:

Code:

List::iterator List::begin() {return List::iterator(m_head);}
List::iterator List::end() {return List::iterator(0);}

In order for the list to be able to erase a node given an iterator, it indeed needs to find out what node the iterator is pointing to. So friendship is ok.

You might try to move the Node to the private section of the List: the user doesn't need to know about that.

Originally Posted by CornedBee

The Node inner class should be private.

You are totally correct. Thanks
One more little issue if i have

Code:

class LinkedList
{
public:
	friend class Iterator;
	
	class Iterator
	{
	public:
		friend class LinkedList;

		// constructors
		Iterator(const LinkedList *list);
		~Iterator();
		
		// methods
		void Next();
		void Start();
		bool IsValid();
		Entry * GetValue();
	private:
		const LinkedList *m_list;		// the list we're iterating
		LinkedList::Node *m_current;	// current node we're in

	};


	// constructors
	LinkedList(void);
	~LinkedList(void);

	// metods
	void Append(const Entry val);
	void Prepend(const Entry val);
	void RemoveHead();	
	void RemoveTail();
	void RemoveNode(Iterator *it);
	Entry Head();

private:
	struct Node
	{
	public:
		Node(Entry val);

		Node();

		Node *next;
		Node *previous;

		Entry val;
	};

	Node *m_head;	// head of the linked list
	Node *m_tail;	// tail fo the linked list

	LinkedList(const LinkedList &){};	// enforce that the user will not pass by value a linked list

};

this fails because in the nested class Iterator i use a Node, and the compiler only sees the declaration of the inner struct Node after the declaration of Iterator. I know this is solved with forward declarations, or by declaring the Node struct before the iterator class.

however how do i foward declare the Node struct;

1º before the LinkedList class like

Code:

struct LinkedList::Node; //this was my 1º though but it fails
struct Node; also fails

one way it doesn't fail is if i declare inside of the LinkedList class like

Code:

public:
	struct Node;
...
...

however i need to forward declare it in a private section or i would get the warning

"Warning 1 warning C4240: nonstandard extension used : access to 'LinkedList::Node' now defined to be 'private', previously it was defined to be 'public'"

but if i declare it in a private section im better of doing

Code:

class LinkedList
{
private:
  struct Node {....};
...
public
   class Iterator {.....};
etc...

This is more a syntax question however how to forward declare the struct Node ?

Once more thanks for all feedback

Just do it the last way.

But if you wanted to forward declare it, you would have to make sure that the forward declaration and the definition have the same visibility.

Alternatively, you could define it out-of-line:

Code:

class List
{
  struct Node;
};

struct List::Node
{
};