Operations on a vector

Question

I have the following class which is used for operations on a vector. The vector stores the following struct which has 3 enums and 1 boolean.

Info represents a computer node. Nodes communicate with each other. Some nodes are unique (in which case they don't have an ID (and others are not so they have a id associated with them. InfoIdEnum basically is {one,two,three,four}. When nodes send each other a message, the info_discovered becomes true. Also, some objects send others information when their state changes. Not all objects talk to each other.

struct Info
{
 InfoEnum info_name;
 InfoIdEnum info_id;
 InfoStatesEnum info_state;
 bool info_discovered = false;
};

The operations on the vector include checking if info_discovered was received for an element having the same info_name and info_id and also returning the InfoStatesEnum for the same case.

I wrote the following functions to implement this:

///Second is optional parameter. Some InfoEnum don't need an ID with them
bool findInfoElement(Info * info,const InfoEnum info_name,const InfoIdEnum info_id=InfoIdEnum::NONE) const 
{
 ///m_info_vector is the vector
 if(m_info_vector.empty())
 {
 return false;
 }
 /// Iterate through elements to find a match
 for(Info node_info : m_info_vector)
 {
 if(node_info.info_name == info_name &&
 node_info.info_id == info_id )
 {
 *info = node_info;
 return true;
 }
 }
 return false;
}

And this is where it is used in the same class:

bool isNodeDiscovered(const InfoEnum name,const InfoIdEnum id=InfoIdEnum::NONE) const
{
 Info node;
 if(findInfoElement(&node, name,id))
 {
 if(node.info_discovered)
 {
 return true;
 }
 }
 return false;
}

And to add a new element if it doesn't exist and update the existing if it exists:

void updateInformation(const Info node_info)
{
 if(m_info_vector.empty())
 {
 m_info_vector.push_back(node_info);
 return;
 }
 /// Iterate through nodes to find a match
 for(Info info : m_info_vector)
 {
 if(info.info_name == node_info.info_name &&
 info.info_id == node_info.info_id )
 {
 /// Node already present,update the values
 info = node_info;
 return;
 }
 }
 ///Not Empty but doesn't contain the value.
 m_info_vector.push_back(node_info);
}

I am worried if there is any potential pitfall I should be aware of in the code / way to optimize it for performance if the vector size is guaranteed to be fewer than 16 elements.

• 200_success: It's hard to tell what you are trying to achieve with this code. Is this real code from one of your projects? If so, could you explain more what an Info represents, and show us the entire class?
• SteveIrwin: that's all the class contains.3 functions and a private m_info_vector.
• 200_success: Then can you edit the question to clarify what an Info represents?
• SteveIrwin: Info represents an object.Objects communicate with each other .Some objects are unique(in which case they don't have an Id) and others are not so they have a id associated with them. InfoIdEnum basically is {one,two,three,four}. When objects send each other a message, the info discovered becomes true.Also some objects send others information when their state changes. Not all objects talk to each other.

Answer 1

Names

Just for example, Info is much too generic a name. Although you haven't said much about what exact sort of info this is, you should rename it to reflect the kind of info being stored.

Data Structures

As long as you're sure you'll never store more than 16 items, I suppose it's reasonable to use a vector and use linear searching to find items. If there's any possibility of more items, I'd switch to something like a map or set. Even as it stands now, I'd consider using a map or unordered_map anyway, simply because it directly supports so many of the things you want/need to do.

Top-level const:

Consider the parameters to your isNodeDiscovered:

bool isNodeDiscovered(const InfoEnum name,const InfoIdEnum id=InfoIdEnum::NONE)

You're passing these by value, so the const doesn't really mean or accomplish anything and might as well be removed:

bool isNodeDiscovered(InfoEnum name, InfoIdEnum id=InfoIdEnum::NONE)

Note that this is not the case when the const applies to something to which you're passing a reference or pointer. There the const can mean a lot and shouldn't be removed with impunity.

Encapsulation

Since you have a specific idea of how one Info object should be compared to another for equality, I'd encode that into the Info object itself:

bool Info::operator==(Info const &other) const { 
 return info_name == other.info_name && info_id == other.info_id;
}

This lets you use standard algorithms to implement most of your other operations:

bool find_info_element(Info *info, InfoEnum info_name, InfoIdEnum info_id=InfoIdEnum::NONE) {
 Info i { info_name, info_id};
 auto pos = std::find(m_info_vector.begin(), m_info_vector.end(), i);
 if (pos == m_info_vector.end())
 return false;
 *info = *pos;
 return true;
}

I'd at least consider changing this to just return the iterator though--that would be much more in keeping with how things are typically done in C++.

Using map

As mentioned previously, using std::map (or unordered_map) would simplify the code quite a bit--to the point that I'm tempted to recommend using it, even though it's sort of overkill for just 16 elements. To do this, we'd start by separating an Info into the part that acts as the key and the data that's stored with that key:

typedef std::pair<InfoEnum, InfoIdEnum> Key;
class Data {
 InfoStatesEnum info_state;
 bool info_discovered = false;
};

Then we'd define a map of that type:

std::map<key, data> info_data;

Then (for example) updateInformation would become something like this:

void updateInformation(Key const &key, Data const &data)
{
 info_data[key] = data;
}

Another possibility would be to use a class that provided the same interface as a map, but stored its data in a vector. For one example, Boost's flat_map does exactly this.

• SteveIrwin: Thanks for the excellent feedback. Using Iterators and refactoring to use Maps sounds a lot better than my current implementation.