Re: What's wrong with this class

From:

Victor Bazarov <v.Abazarov@comAcast.net>

Newsgroups:

comp.lang.c++

Date:

Fri, 24 Oct 2008 10:43:12 -0400

Message-ID:

<gdsmu2$37a$1@news.datemas.de>

John Doe wrote:

Hi,

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'?

{
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...

~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; }

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) {}

? 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;
    }

    void Clear()
    {
        for_each( _netlist.begin(), _netlist.end(), DeletePointer ());
    }

    void Add(CNetwork* network)
    {
        _netlist.push_back(network);
    }

Now, you do realise that your list is made the owner of that pointer
here, right?

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:

const CNetwork* getNetwork(CStirng const& netNameOrGuid) const

    {
        if ((netNameOrGuid.GetAt(0) == '{') &&
            netNameOrGuid.GetLength() == 39)
        {
            CLSID guid;
            if
(SUCCEEDED(CLSIDFromString(netNameOrGuid.GetBuffer(),&guid)))
                return getNetwork(guid);
        }
        else
        {
            NetworkListIt it;
            for (it = _netlist.begin(); it != _netlist.end(); ++it)
                if (!(*it)->getName().CompareNoCase(netNameOrGuid))
                    return (*it);
        }
        return NULL;
    }

    const CNetwork* getNetwork(CLSID guid)

Same comment as above,

const CNetwork* getNetwork(CLSID guid) const

    {
        if (!_netlist.empty())
            Clear();

        NetworkListIt it;
        for (it = _netlist.begin(); it != _netlist.end(); ++it)
            if ((*it)->getGuid() == guid)
                return (*it);

        return NULL;
    }

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.

            {
                netList.Add(pNetWork);
            }

            i++;
        }
}

When I call this code :
m_NetworkList = getNetworkList();

Where?

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.

Post complete code and provide a test driver that would produce the
network (instead of 'ConnMgrEnumDesitnations' which C++ doesn't have).

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask