Composite Pattern / Gui, proof-read plz

Started by
8 comments, last by Janju 18 years, 9 months ago
/* gui.h */
#ifndef GUI_H
#define GUI_H
#include <vector>

class Gui
{
public:
	class IDrawable
	{
		std::vector<IDrawable*>childs;
		std::vector<IDrawable*>::iterator j;

		virtual void drawChilds()
		{
			j = childs.begin();
			while (j != childs.end())
			{
				(*j)->draw();
				j++;
			}
		}

	public:
		virtual void add(IDrawable * d)
		{
			//j = childs.begin();
			childs.push_back(d);
		}

		virtual void clear()
		{
			j = childs.begin();
			while (j != childs.end())
			{
				(*j)->clear();
				delete(*j);
				j++;
			}
			childs.clear();
		}		

		virtual void draw()
		{
			drawChilds();
		};

		IDrawable()
		{
			j = childs.begin();
		}

		virtual ~IDrawable()
		{
			j = childs.begin();
			while (j != childs.end())
			{
				delete(*j);
				j++;
			}
			childs.clear();
		}		
	};

private:
	static std::vector<IDrawable*>childs;
	static std::vector<IDrawable*>::iterator j;

public:
	static void add(IDrawable * d)
	{
		//j = childs.begin();
		childs.push_back(d);
	}

	static void clear()
	{
		j = childs.begin();
		while (j != childs.end())
		{
			(*j)->clear();
			delete(*j);
			j++;
		}
		childs.clear();
	}		

	static void drawOnce()
	{
		j = childs.begin();
		while (j != childs.end())
		{
			(*j)->draw();
			j++;
		}

		clear();
	};	
};

#endif

/* gui.cpp */
#include "gui.h"

std::vector<Gui::IDrawable*> Gui::childs;
std::vector<Gui::IDrawable*>::iterator Gui::j;





Will the clear function do what it should? Or will it leave memory leaks?
Advertisement
I'd say it will work.
Lead programmer of the (slowly evolving) syBR game engine --- http://sybr.sourceforge.net
Quote:Original post by sebarnolds
I'd say it will work.
Is the destructor of the IDrawable correct? Or do i have to add (*j)->clear() to it?

or would this be better:?
virtual ~IDrawable()		{			clear();		}
That doesn't look like a typical instance of the composite pattern to me, i also think the behavior of IDrawable::clear to be abit to drasitic, also calling std::vector::clear inside of IDrawable's destructor is unnecessary.

I suggest taking another look at the composite pattern and i also suggest using smart pointers instead like boost/std::tr1::shared/weak_ptr.
Quote:Original post by snk_kid
That doesn't look like a typical instance of the composite pattern to me, ...
Well it's not 100% composite, but pretty close.

Quote:Original post by snk_kid
... i also think the behavior of IDrawable::clear to be abit to drasitic,...
I build up the whole gui every frame. It's ok since my gui will be fairly small and easy.

Quote:Original post by snk_kid
... also calling std::vector::clear inside of IDrawable's destructor is unnecessary.
Can you explain?

Quote:Original post by snk_kidI suggest taking another look at the composite pattern and i also suggest using smart pointers instead like boost/std::tr1::shared/weak_ptr.
I don't know how i can get boost to run with my vs2003 and i don't know if i even need it's features atm.
Well the only thing that I'm wondering is do you want the Children objects destoryed or not?

Cause if you do then in the Clear method you don't need another call to clear() since delete will call the Destructor in which calls the clear method so the (*j)->clear() in the IDrawable::clear() method is redundant so get ride of it.

If you don't want the objects deleted but just want to remove them as children then I'd suggest only doing a childs.clear() to delete all the pointer in the vector like you have at the end. Remember if you don't have another reference to the children within the IDrawable class that your calling clear() on and don't delete them you will have memory leaks.

Oh and isn't it supposed to be 'children' not 'childs' and with your current code you don't need to initialize j to childs.begin() in the constructor.

I hope that helped a little bit!
Quote:Original post by Jonus
Quote:Original post by snk_kid
That doesn't look like a typical instance of the composite pattern to me, ...
Well it's not 100% composite, but pretty close.


Not really, i think your missing the point of the composite pattern, look at the structure of the composite and your one where you have all node type as nonterminal nodes in a tree, see the advantage of the composite's structure over your one. i'll explain further at the end with a code example.

Quote:Original post by Jonus
Quote:Original post by snk_kid
... also calling std::vector::clear inside of IDrawable's destructor is unnecessary.
Can you explain?


std::vector's destructor will be invoked thus already destroying and deallocating memory so a call to std::vector::clear in IDrawable's destructor is just redundant.

Quote:Original post by Jonus
Quote:Original post by snk_kidI suggest taking another look at the composite pattern and i also suggest using smart pointers instead like boost/std::tr1::shared/weak_ptr.
I don't know how i can get boost to run with my vs2003 and i don't know if i even need it's features atm.


Its very simple to build boost for VC++ 2003 follow this its matter of one simple command for VC++ 7.1 and waiting for it to build. Besides you don't have to use boost you could write your own or use another smart pointer package but you will thank me or someone else later if you do have boost for other things.

I have further suggestions;


  • prefer using generic algorithms over hand-written loops on containers

  • i'd use std::deque with boost::fast_pool_allocator as std::deque is better than std::vector at growing, std::deque is also random access container. You don't need contigious-ness of elements in memory that std::vector gives plus you will never know for sure exactly how much to reserve in advance anyways hence std::deque of (smart) pointers will much better serve your needs (especially with a custom allocator type such as boost::fast_pool_allocator)

  • You could have an embedded reference count in your widget hierarchy then use something like boost::intrusive_ptr instead of shared_ptr to improve preformance further but as the name suggests its an intrusive solution

  • I recommend you use a generic signals & slots framework for event handling such as sigslot or Boost.Signals its definitely the better methods to do it in C++



With that in mind an example of some of the things i suggested.

#include <deque>#include <boost/pool/pool_alloc.hpp>#include <boost/shared_ptr.hpp>#include <boost/bind.hpp>struct bounds {};struct widget {	typedef boost::shared_ptr<widget> widget_ptr;private:	friend struct widget_container;	widget_container* parent_;	bounds bv_;	virtual widget* do_clone() const = 0;protected:	widget(widget_container* parent = 0): parent_(parent), bv_() {}	widget(const widget& w): parent_(w.parent_), bv_(w.bv_) {}	widget& operator=(const widget& w); // disable assignement	void set_parent(widget_container* wc) {		parent_ = wc;	}public:	widget_ptr clone() const {		widget_ptr p(do_clone());		return p;	}	//......	virtual ~widget() {}};struct widget_container : widget {	typedef boost::fast_pool_allocator<widget_ptr> widget_ptr_alloc;	typedef std::deque<widget_ptr, widget_ptr_alloc> child_list;	typedef child_list::size_type size_type;private:	child_list kids;	virtual widget_container* do_clone() const {		return new widget_container(*this);	}protected:	widget& operator=(const widget& w); // disable assignement	widget_container(const widget_container& wc)	: widget(wc), kids() {				std::transform(wc.kids.begin(), wc.kids.end(),						std::back_inserter(kids), boost::bind(&widget::clone, _1));	}	public:	widget_container(widget_container* p = 0)	: widget(0), kids() {}	void add_child(const widget_ptr& w) {		w->set_parent(this);		kids.push_back(w);	}	size_type size() const {		return kids.size();	}	//....};


Remember this is a simplified example but that is the kind of structure you want. Once you have that all you do is derive your leaf node types (such as button etc) from widget and all your composite types those types that are widgety containers and represent internal nodes of tree from widget_container
Quote:Original post by snk_kid
Quote:Original post by Jonus
Quote:Original post by snk_kid
That doesn't look like a typical instance of the composite pattern to me, ...
Well it's not 100% composite, but pretty close.


Not really, i think your missing the point of the composite pattern, look at the structure of the composite and your one where you have all node type as nonterminal nodes in a tree, see the advantage of the composite's structure over your one. i'll explain further at the end with a code example.
What does "where you have all node type as nonterminal nodes in a tree" mean?

Imho my composite is fairly close to what is thought in the book of the gang of 4:
/** * composite.h * Implemented by Blueprint Technologies, Inc. */#ifndef _composite_h#define _composite_h#include <vector>using namespace std;/** * Declares the interface for objects in the composition. Implements * default behavior for the interface common to all classes, as  * appropriate. Declares an interface for accessing and managing its * child components. */class Component{public:	virtual void operation() = 0;	virtual void add( Component* )	{	};	virtual void remove( Component* )	{	};	virtual Component* getChild( int )	{		return (Component *) 0;	};};/** * Represents leaf objects in the composition. A leaf has no children. * Defines behavior for primitive objects in the composition. */class Leaf: public Component{public:	virtual void operation()	{		// do something here.	};};/** * Defines behavior for components having children. Stores child components. * Implements child-related operations in the Component interface. */class Composite: public Component{private:	vector< Component* > children;public:	virtual void operation()	{		for( int x = 0; x < children.size(); ++x )		{			Component* component = children[x];			component -> operation();		}	};	virtual void add( Component* obj )	{		children.push_back( obj );	};	virtual void remove( Component* obj )	{		vector< Component* >::iterator i;		int found;		while( 1 )		{			found = 0;			for( i = children.begin(); i != children.end(); ++i )			{				if( *i == obj )				{					found = 1;					children.erase( i );					break;				}			}			if( found == 0 )			{				break;			}		}	};	virtual Component* getChild( int index )	{		return children[ index ];	};};#endif
I neither need leafs nor remove single childs in my small gui. Furthermore I don't need to modify the children so there is no need for the getChild member in my composite.

Quote:Original post by snk_kid
Quote:Original post by Jonus
Quote:Original post by snk_kid
... also calling std::vector::clear inside of IDrawable's destructor is unnecessary.
Can you explain?


std::vector's destructor will be invoked thus already destroying and deallocating memory so a call to std::vector::clear in IDrawable's destructor is just redundant.
Well but the destructor of the vector will only deallocate the pointers to the components not the components themself or am i wrong?

Quote:Original post by snk_kid
Quote:Original post by Jonus
Quote:Original post by snk_kidI suggest taking another look at the composite pattern and i also suggest using smart pointers instead like boost/std::tr1::shared/weak_ptr.
I don't know how i can get boost to run with my vs2003 and i don't know if i even need it's features atm.


Its very simple to build boost for VC++ 2003 follow this its matter of one simple command for VC++ 7.1 and waiting for it to build. Besides you don't have to use boost you could write your own or use another smart pointer package but you will thank me or someone else later if you do have boost for other things.

I have further suggestions;


  • prefer using generic algorithms over hand-written loops on containers

  • i'd use std::deque with boost::fast_pool_allocator as std::deque is better than std::vector at growing, std::deque is also random access container. You don't need contigious-ness of elements in memory that std::vector gives plus you will never know for sure exactly how much to reserve in advance anyways hence std::deque of (smart) pointers will much better serve your needs (especially with a custom allocator type such as boost::fast_pool_allocator)

  • You could have an embedded reference count in your widget hierarchy then use something like boost::intrusive_ptr instead of shared_ptr to improve preformance further but as the name suggests its an intrusive solution

  • I recommend you use a generic signals & slots framework for event handling such as sigslot or Boost.Signals its definitely the better methods to do it in C++



With that in mind an example of some of the things i suggested.

*** Source Snippet Removed ***

Remember this is a simplified example but that is the kind of structure you want. Once you have that all you do is derive your leaf node types (such as button etc) from widget and all your composite types those types that are widgety containers and represent internal nodes of tree from widget_container
As much as a appreciate your time for writing all that stuff up, it won't help me much. I think including boost in my simple programm will make it overly complicated to understand for everyone else. I don't know what a boost::fast_pool_allocator or boost::intrusive_ptr is and i don't want to waste my whole time on learning things that i don't need at all to complete my stuff.

Of course i want to try out boost regardless of my current project, but i fail to see what that single command is i need to put in that vc7.1 can run boost.

When i will do my next programm and write a real gui, i will try to get working what you suggest here. I will save this page for later. Thanks.
Quote:Original post by Jonus
Well but the destructor of the vector will only deallocate the pointers to the components not the components themself or am i wrong?


No your wright and std::vector::clear wont either, what i mean with this:

virtual ~IDrawable() {   j = childs.begin();   while (j != childs.end())   {	delete(*j);	j++;   }   childs.clear();}


That bit in bold isn't really needed because vector's destructor will destroy & deallocate its own memory (just the pointers) and it will still be the case for all children and chilren's children because you are deleteing the children your self which will eventually invoke the destructor of std::vector in the children and children's children etc.
Quote:Original post by snk_kid
Quote:Original post by Jonus
Well but the destructor of the vector will only deallocate the pointers to the components not the components themself or am i wrong?


No your wright and std::vector::clear wont either, what i mean with this:

virtual ~IDrawable() {   j = childs.begin();   while (j != childs.end())   {	delete(*j);	j++;   }   childs.clear();}


That bit in bold isn't really needed because vector's destructor will destroy & deallocate its own memory (just the pointers) and it will still be the case for all children and chilren's children because you are deleteing the children your self which will eventually invoke the destructor of std::vector in the children and children's children etc.
Ok. I rethought everything you and 5MinuteGaming said here is the result:
#ifndef GUI_H#define GUI_H#include <windows.h>		// Header File For Windows#include <math.h>#include <gl\gl.h>			// Header File For The OpenGL32 Library#include <gl\glu.h>			// Header File For The GLu32 Library#include <gl\glaux.h>		// Header File For The Glaux Library#include <vector>//#include "simplefont.h"class Gui{public:	class IDrawable	{		std::vector<IDrawable*>childs;		std::vector<IDrawable*>::iterator j;		virtual void drawChilds()		{			j = childs.begin();			while (j != childs.end())			{				(*j)->draw();				j++;			}		}	public:		virtual void add(IDrawable * d)		{			//j = childs.begin();			childs.push_back(d);		}			virtual void draw()		{			drawChilds();		};		IDrawable()		{			j = childs.begin();		}		virtual ~IDrawable()		{			j = childs.begin();			while (j != childs.end())			{				delete(*j);				j++;			}		}			};private:	static std::vector<IDrawable*>childs;	static std::vector<IDrawable*>::iterator j;public:	static void add(IDrawable * d)	{		//j = childs.begin();		childs.push_back(d);	}	static void clear()	{		j = childs.begin();		while (j != childs.end())		{			delete(*j);			j++;		}		childs.clear();	}			static void draw()	{		j = childs.begin();		while (j != childs.end())		{			(*j)->draw();			j++;		}	};	};#endif
Thanks :).

This topic is closed to new replies.

Advertisement