What's wrong with this class

[Thomas J. Gritzan]

Re: What's wrong with this class

John Doe schrieb:

Victor Bazarov wrote:

>John Doe wrote:

>
So here is the update code (without the try for new I will do it later)
but now I get an error :
error C2662: 'NetworkList::C lear' : cannot convert 'this' pointer from
'const NetworkList' to 'NetworkList &'
>
so I have added const to Clear()...

But Clear changes the class, at least logically, so making Clear() const
is wrong. Why do you need the function anyway?

//=============== =============== ==============//
// NetworkManager. h
//=============== =============== ==============//
struct DeletePointer {
template<typena me T>
void operator()(cons t T* ptr) const
{
delete ptr;
}
};
>
class Network
{
public:
Network() {};
Network(CString const& netName, GUID netguid):
_netname(netNam e), _netguid(netgui d) {}
>
~Network() {}

You don't need to define an empty destructor.

CString const& getName() { return _netname; }

getName should be const.

GUID getGuid() const { return _netguid; }
>
private:
CString _netname;
GUID _netguid;
};
>
>
class NetworkList
{
typedef std::list<Netwo rk*>::iterator NetworkListIt;
>
public:
>
NetworkList()
{
}
>
~NetworkList()
{
Clear();
}
>
void Clear()
{
for_each( _netlist.begin( ), _netlist.end(), DeletePointer ());
}

Clear deletes the objects in your list, but you don't clear the list
itself. You have dangling pointers.

[...]

const Network* getNetwork(CLSI D guid) const
{
if (!_netlist.empt y())
Clear();

Here you call delete on all your pointers, but you don't clear _netlist.
After that, you use these dangling pointers, which is undefined
behaviour and most likely will crash your program.

Why do you clear the list just before you use it?

std::list<Netwo rk*>::const_ite rator it;
//NetworkListIt it;
for (it = _netlist.begin( ); it != _netlist.end(); ++it)
if ((*it)->getGuid() == guid)
return (*it);
>
return NULL;
}
>
private:
std::list<Netwo rk*_netlist;
};

Just as the other posters suggested, the easiest is not to use pointers
at all and change the member variable to:

std::list<Netwo rknetlist; // std::vector would do it as well.

--
Thomas

[James Kanze]

Re: What's wrong with this class

On Oct 25, 12:00 am, Mosfet <mos...@anonymo us.orgwrote:

James Kanze a écrit :

Actually I am reading effective STL and there is a chapter
where is written :
Containers will make a copy of the object that you insert.
This is fine when inserting integers and strings, but for
large/complex objects becomes undesirable. If you do not want
multiple copies of the object floating around, store pointers
in containers rather than actual objects. If you are happy
with multiple copies, get to know your copy-constructor.

Usually I use object and not pointers but I told me that I
could try with pointers. OF course there are some issues with
pointer ownerships so I will do as I always do and will do
some advanced tricks only in a few years ;-)

There is one idiom that might be applicable here: make the
contained objects immutable, and use boost::shared_p tr to manage
them. (Or better yet, develop your own invasive reference
counted pointer, based on e.g. the one in item 29 of "More
Effective C++".) This basically duplicates the way you'd handle
a value type in Java, with two restrictions: your value objects
can't contain pointers to other objects which might create
cycles, and it isn't thread safe. And it generally isn't worth
the bother, unless the objects are very, very expensive to copy
(not your case).

--
James Kanze (GABI Software) email:james.kan ze@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientier ter Datenverarbeitu ng
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34