How to make iterating through list of variables more elegant

[Angus]

Hello

I have the following string variables in my class:

m_PrimaryID, m_SecondaryID and m_TertiaryID.

I have a search function, FindTool(), which goes through each of these
ID's to search for something.

I have a getID() function like this:
const char* CTool::getID(int index) const
{
switch(index)
{
case 0: return m_PrimaryID && *m_PrimaryID ? m_PrimaryID : 0;
case 1: return m_SecondaryID && *m_SecondaryID ?
m_SecondaryID : 0;
case 2: return m_TertiaryID && *m_TertiaryID ? m_TertiaryID :
0;
default: return 0;
}
}

which returns the requisite variable if not null.

I do the search like this:
CTool* CTool::FindTool()
{
CTool* tool = 0;
//try all available gcid's
for(int it = 0; it != 3; ++it)
{
if(getID(it))
tool = findTool(getID(it)); //this is using library I need
to use

if(tool && tool->Exists())
return tool;
}

return 0;
}

But I am concerned the code looks obscure. How can I make it more
elegant?

Angus

 

[Francesco S. Carta]

Hello

I have the following string variables in my class:

m_PrimaryID, m_SecondaryID and m_TertiaryID.

I have a search function, FindTool(), which goes through each of these
ID's to search for something.

I have a getID() function like this:
const char* CTool::getID(int index) const
{
switch(index)
{
case 0: return m_PrimaryID&& *m_PrimaryID ? m_PrimaryID : 0;
case 1: return m_SecondaryID&& *m_SecondaryID ?
m_SecondaryID : 0;
case 2: return m_TertiaryID&& *m_TertiaryID ? m_TertiaryID :
0;
default: return 0;
}
}

which returns the requisite variable if not null.

I do the search like this:
CTool* CTool::FindTool()
{
CTool* tool = 0;
//try all available gcid's
for(int it = 0; it != 3; ++it)
{
if(getID(it))
tool = findTool(getID(it)); //this is using library I need
to use

if(tool&& tool->Exists())
return tool;
}

return 0;
}

But I am concerned the code looks obscure. How can I make it more
elegant?

Angus

Well, at least to me, it looks a bit obscure, but it's just matter of
style - that's the ternary "?:" what makes it hard to read (for me).

One option could be:

//-------
const char* CTool::getID(int index) const {
if(index == 1 && m_PrimaryID && *m_PrimaryID) {
return m_PrimaryID;
}
if(index == 2 && m_SecondaryID && *m_SecondaryID) {
return m_SecondaryID;
}
if(index == 3 && m_TertiaryID && *m_TertiaryID) {
return m_TertiaryID;
}
return 0;
}
//-------

Or its equivalent version using the switch along with appropriate breaks.

I think your design can be improved further than that, but it's again
matter of style/tastes and you didn't post enough code to get wider
suggestions.

Well, I might add that you could store those pointers in a vector and
access them as "m_ID[0]" (or "m_ID[1]") instead of "m_PrimaryID", so
that you can automate the selection process without having to duplicate
code that differs just in the variable's names... never mind, you'll
eventually post further questions (and code) if you're interested in
such a change.

 

[Alf P. Steinbach /Usenet]

* Angus, on 12.07.2010 18:56:

Hello

I have the following string variables in my class:

m_PrimaryID, m_SecondaryID and m_TertiaryID.

I have a search function, FindTool(), which goes through each of these
ID's to search for something.

I have a getID() function like this:
const char* CTool::getID(int index) const
{
switch(index)
{
case 0: return m_PrimaryID&& *m_PrimaryID ? m_PrimaryID : 0;
case 1: return m_SecondaryID&& *m_SecondaryID ?
m_SecondaryID : 0;
case 2: return m_TertiaryID&& *m_TertiaryID ? m_TertiaryID :
0;
default: return 0;
}
}

which returns the requisite variable if not null.

I do the search like this:
CTool* CTool::FindTool()
{
CTool* tool = 0;
//try all available gcid's
for(int it = 0; it != 3; ++it)
{
if(getID(it))
tool = findTool(getID(it)); //this is using library I need
to use

if(tool&& tool->Exists())
return tool;
}

return 0;
}

But I am concerned the code looks obscure. How can I make it more
elegant?

CTool::getID implements array indexing. So for that aspect, simply put the id's
in an array. I suggest a std::vector, unless the limit of 3 is really firm.

Then you have this testing of pointer nullness. Use std::string instead of char*.

The call to findTool, within the index loop, might then look like

if( id != "" )
tool = findTool( id.c_str() );


Cheers & hth.,

- Alf

 

[Victor Bazarov]

I have the following string variables in my class:

m_PrimaryID, m_SecondaryID and m_TertiaryID.

Apparently, those are pointers to char or something convertible to a
pointer to char... It would be nice if you spelled it out for us,
instead of making us deduce this. Like so:

class CTool // or does it inherit from something?
{
char* m_PrimaryID;
char* m_SecondaryID;
char* m_TertiaryID;
// other stuff irrelevant
public:
const char* getID(int) const;
CTool* findTool(const char*); // or is it inherited?
CTool* FindTool();
bool Exists();
// other interface not important
};

I have a search function, FindTool(), which goes through each of these
ID's to search for something.

I have a getID() function like this:
const char* CTool::getID(int index) const
{
switch(index)
{
case 0: return m_PrimaryID&& *m_PrimaryID ? m_PrimaryID : 0;
case 1: return m_SecondaryID&& *m_SecondaryID ?
m_SecondaryID : 0;
case 2: return m_TertiaryID&& *m_TertiaryID ? m_TertiaryID :
0;
default: return 0;
}
}

which returns the requisite variable if not null.

I do the search like this:
CTool* CTool::FindTool()
{
CTool* tool = 0;
//try all available gcid's
for(int it = 0; it != 3; ++it)
{
if(getID(it))
tool = findTool(getID(it)); //this is using library I need
to use

"Need" as in "must"? Or "need" as in "has the functionality I don't
want to reinvent"? Is that a member of the base class? Is that a
global function? Consider supplying 'this->' in the former case and the
namespace prefix '::' in the latter.

You can declare a local variable instead of calling 'getID' twice:

if (const char *cid = getID(it))
tool = findTool(cid);

Is that more elegant? I think so.

if(tool&& tool->Exists())
return tool;
}

return 0;
}

But I am concerned the code looks obscure. How can I make it more
elegant?

Naming functions better might help (you have both 'getID' and
'FindTool', which are mixing different case conventions - bad). Adding
comments would certainly make the code more readable (and thus less
obscure)... Prefixes to the function to avoid misinterpreting global
functions as members (when read by a human, apparently the compiler has
no problem). Better control of the whitespace and indentation would help...

Anyway, what's your definition of "elegant"? Is that "I know one when I
see it"?

V

 

[Francesco S. Carta]

Hello

I have the following string variables in my class:

m_PrimaryID, m_SecondaryID and m_TertiaryID.

I have a search function, FindTool(), which goes through each of these
ID's to search for something.

I have a getID() function like this:
const char* CTool::getID(int index) const
{
switch(index)
{
case 0: return m_PrimaryID&& *m_PrimaryID ? m_PrimaryID : 0;
case 1: return m_SecondaryID&& *m_SecondaryID ?
m_SecondaryID : 0;
case 2: return m_TertiaryID&& *m_TertiaryID ? m_TertiaryID :
0;
default: return 0;
}
}

which returns the requisite variable if not null.

I do the search like this:
CTool* CTool::FindTool()
{
CTool* tool = 0;
//try all available gcid's
for(int it = 0; it != 3; ++it)
{
if(getID(it))
tool = findTool(getID(it)); //this is using library I need
to use

if(tool&& tool->Exists())
return tool;
}

return 0;
}

But I am concerned the code looks obscure. How can I make it more
elegant?

Or, wait, were you concerned about "CTool::FindTool()"?

That function doesn't look obscure to me.