Re: What's wrong with this class
On Oct 24, 4:43 pm, Victor Bazarov <v.Abaza...@comAcast.net> wrote:
John Doe wrote:
I wrote a small class to enumerate available networks on a
smartphone :
class CNetwork
Why do you need the 'C' in front of the name? I can
understand 'SP' (for smartphone), but 'C'?
I thought C was the prefix Microsoft reserved for MFC. With
namespaces, of course, you don't need such things at all
(although maybe the class should be in a SmartPhone namespace).
{
public:
CNetwork() {};
CNetwork(CString& netName, GUID netguid):
_netname(netName), _netguid(netguid) {}
If your class owns the member '_netname' (which it probably
should, as you designed it), consider passing the initialiser
for it as a reference to const:
CNetwork(CString const& netName, GUID...
Well, I suppose it really depends on what CString is. But I
think this one is from MFC, and that it has basic copy
semantics, much like std::string (which he should probably
consider using, instead of CString), then the most natural form
is call by value. Of course, if he doesn't use call by value or
a const reference, he can't construct from anything but an
lvalue---no CString returned from a function, for example.
~CNetwork() {}
CString& getName() { return _netname; }
This is a bad idea. You are exposing the innards of your
class to any change that you can't control or even know about.
If your 'Network' needs to report its name, this member has
to be 'const' and should return a reference to const:
CString const& getName() cosnt { return _netname; }
Again, returning a value is more natural. In this case,
returning even a const reference is dangerous, because it only
really works if the client copies it immediately (in which case,
RVO would have eliminated the copy in return by value);
otherwise, you have a potential problem with the lifetime of the
reference.
A string is a value; it's almost always wrong to have a
reference or a pointer to one.
Of course, making the function const is a good idea.
GUID getGuid() { return _netguid; }
Same here:
GUID getGuid() const { return _netguid; }
private:
CString _netname;
GUID _netguid;
};
class CNetworkList
{
public:
typedef std::list<CNetwork*>::iterator NetworkListIt;
Does this need to be public?
CNetworkList() {}
~CNetworkList()
{
Clear();
}
CNetworkList::CNetworkList(const CNetworkList& rhs) {
CopyObj(rhs);
}
What is that? Why couldn't you just use the normal copy
constructor form:
CNetworkList::CNetworkList(const CNetworkList& rhs) :
netlist(rhs.netlist) {}
?
Because the semantics aren't correct. His list is a list of
pointers (which is probably the real error), and he needs to
copy the pointed to objects, or strange things are goind to
happen.
And if it matters, this is pretty much unnecessary because
the compiler will provide you with the copy constructor that
does exactly that.
CNetworkList& CNetworkList::operator=(const CNetworkList& rhs=
)
{
CopyObj(rhs);
return *this;
}
Again, the assignment operator provided by the compiler will work just
fine, most likely. You don't have to provide your own.
void CopyObj(const CNetworkList& rhs)
{
_netlist = rhs._netlist;
}
Except, of course, he got it wrong here. Given the semantics of
his destructor, if he's using pointers, he needs something like:
_netlist.clear() ;
for ( NetworkListIt iter = rhs._netlist.begin() ;
iter != rhs._netlist.end() ;
++ iter ) {
_netlist.push_back( new CNetwork( **iter ) ) ;
}
Another good reason not to use pointers.
void Clear()
{
for_each( _netlist.begin(), _netlist.end(), DeletePoint=
er ());
With values, rather than pointers, this becomes simply:
_netlist.clear() ;
void Add(CNetwork* network)
{
_netlist.push_back(network);
}
Now, you do realise that your list is made the owner of that
pointer here, right?
That's the real problem. It is as far as the destructor's
concerned. But not according to the copy constructor nor the
assignment operator. It's not surprising that he's having
problems.
Again, CNetwork is a value, not an entity (at least from what
little I can see here). That means: no pointer to it, and no
references to it. (There are, of course, exceptions, e.g. like
the return value of [] in an std::vector, where you actually
want to allow access to the element, and thus explicitly allow
the client to modify something internal to your data structure.)
If he drops the pointers:
void add( CNetwork newNetwork )
{
_netlist.push_back( newNetwork ) ;
}
and it works.
const CNetwork* getNetwork(CString netNameOrGuid)
The interface of this function is better if (a) it's 'const'
and (b) its argument is not passed by value:
Agreed with (a), but why (b). If CString has value semantics,
it should be treated as a value, and thus passed by value. (I
know the optimization argument, but until he has really
understood this distinction between values and entities, it's
definitely premature optimization.)
[...]
private:
std::list<CNetwork*> _netlist;
};
CNetworkList getNetworkList()
{
int i = 0;
HRESULT hResult;
CNetworkList netList;
while( ConnMgrEnumDestinations( i, &connInfo ) == 0 )
{
CNetwork* pNetWork = new
CNetwork(CString(connInfo.szDescription), connInfo.guid);
if (pNetWork)
'new' never returns NULL. You should, however, surround it
with 'try-catch' since 'new' throws 'std::bad_alloc' if
something happens.
Only if that something is running out of memory. There are a
lot of things you can do which result in undefined behavior.
(Witness this program.)
{
netList.Add(pNetWork);
}
i++;
}
}
When I call this code :
m_NetworkList = getNetworkList();
Where?
Just a guess, but probably in the destructor of the temporary
returned by getNetworkList(). At least, that's the first place
I see off hand with undefined behavior. Basically, he
constructed a list in the netList variable of getNetworkList;
this list assumed ownership of the CNetwork objects it
contained, and deletes them in its destructor (before
returning). Just before deleting them, however, it copies the
list into the temporary which will be returned. At that point,
you have two CNetworkList with the same pointers, which means
that when the destructor of the second one is called, bam.
Double delete, aka undefined behavior.
I got an assert in a Cstring desctructor so I suppose my
class is doing wrong... When I debug in step by step I
don't really understand the calls, it seems Clear() is
called why it shoudn't.
You call it in your destructor. You have a local variable of
CNetworkList type in your function, and you have a temporary
which is returned. That's two calls to Clear() that I see right
off.
Post complete code and provide a test driver that would
produce the network (instead of 'ConnMgrEnumDesitnations'
which C++ doesn't have).
That too. But I'd suggest he start by reading Scott Meyers.
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34