Suspicious NextIdentifier Function

[Edgar]

The function in srccommandsCommandManager.cpp At or near line number 631:

Code: Select all

int CommandManager::NextIdentifier(int ID)
{
   ID++;

   //Skip the reserved identifiers used by wxWidgets
   if((ID >= wxID_LOWEST) && (ID <= wxID_HIGHEST)) 
      ID = wxID_HIGHEST+1;

   return ID;
}

looks suspicious. As of now, I don't think it's ever exercised (wxID_LOWEST Is 4999; wxID_HIGHEST Is 5999). The purpose of the function is to create and return the next unique command ID. Note that if the if clause is executed more than once (if it's going to be executed at all it will probably happen more than once) the same (non-unique) ID will be returned.

After incrementing ID (which in itself is poor programming technique because it is a parameter which is not passed by reference) there should be some kind of while loop which verifies that the new ID is in fact unique and does not impinge on the reserved identifiers. Something vaguely like:

Code: Select all

int CommandManager::NextIdentifier(int ID)
{
   int id = ID + 1;
   bool unique = IsIDUnique ();
   while (! unique) {
      //Skip the reserved identifiers used by wxWidgets
      if((id >= wxID_LOWEST) && (id <= wxID_HIGHEST)) 
         id = wxID_HIGHEST + id + 1;//Maybe something better?
      else
         id++;
      unique = IsIDUnique ();
   }
   return id;
}

I will leave writing bool IsIDUnique () as an exercise for the next person <grin>.

[Edgar]

I guess I ended up being the "next person"…(now) tested code:

Code: Select all

//efm5  start 
bool CommandManager::IDIsUnique(int pID) {
   bool returnValue = false;
   CommandIDHash::iterator IDIterator;
   for(IDIterator = mCommandIDHash.begin(); IDIterator != mCommandIDHash.end(); ++IDIterator) {
      int commandID = IDIterator->first;
      if (pID == commandID) return false;
   }
   return true;
}

int CommandManager::NextIdentifier(int ID)
{
   //ID++;

   ////Skip the reserved identifiers used by wxWidgets
   //if((ID >= wxID_LOWEST) && (ID <= wxID_HIGHEST)) {
   //   ID = wxID_HIGHEST+1;
   //   wxMessageBox (wxT (" NextIdentifier  ID set to highest"));//efm5 debug crash
   //}

   //return ID;   
   int id = ID + 1;
   bool unique = IDIsUnique(id);
   while (! unique) {
      //Skip the reserved identifiers used by wxWidgets
      if((id >= wxID_LOWEST) && (id <= wxID_HIGHEST)) 
         id = wxID_HIGHEST + id + 1;//Maybe something better?
      else
         id++;
      unique = IDIsUnique(id);
   }
   return id;
   //efm5  end 
}