std::map inizialitazion (only one time)

Question

I have a function that translates data using std::map

struct HistoParameter
{
  int nbins;
  float first;
  float last;
  HistoParameter(int _nbins, int _first, int _last) :
    nbins(_nbins), first(_first), last(_last) {};
};

HistoParameter* variable_to_parameter(char* var_name)
{
  std::map<const std::string, HistoParameter*> hp;
  hp[std::string("ph_pt")] = new HistoParameter(100,0,22000);
  hp[std::string("ph_eta")] = new HistoParameter(100,-3,3);
  // ...
  return hp[var_name];
}

My struct is very light, but image it can be heavy. The prolem is that every time I call this function it create a lot of HistoParameter objects, maybe a switch case is more efficient. First question: I'm creating garbage?

Second solution:

bool first_time = true;
HistoParameter* variable_to_parameter(char* var_name)
{
  static std::map<const std::string, HistoParameter*> hp;
  if (first_time)
    {
  hp[std::string("ph_pt")] = new HistoParameter(100,0,22000);
  hp[std::string("ph_eta")] = new HistoParameter(100,-3,3);
  // ...
    }
  first_time = false;
  return hp[var_name];

is it ok? Better solution?

Answer 1

Your second solution should certainly improve efficiency, but isn't (at least IMO) the best implementation possible. First of all, it makes first_time publicly visible, even though only variable_to_parameter actually cares about it. You've already made hp a static variable in the function, and first_time should be as well.

Second, I would not use pointers and/or dynamic allocation for the HistoParameter values. At one int and two floats, there's simply no reason to do so. If you're really passing them around so much that copying became a problem, you'd probably be better off using some sort of smart pointer class instead of a raw pointer -- the latter is more difficult to use and much more difficult to make exception safe.

Third, I'd consider whether it's worthwhile to make variable_to_parameter into a functor instead of a function. In this case, you'd initialize the map in the ctor, so you wouldn't have to check whether it was initialized every time operator() was invoked. You can also combine the two, by have a static map in the functor. The ctor initializes it if it doesn't exist, and operator() just does a lookup.

Finally, I'd note that map::operator[] is primarily useful for inserting items -- it creates an item with the specified key if it doesn't exist, but when you're looking for an item, you usually don't want to create an item. For this, you're generally better off using map.find() instead.

Answer 2

The second solution seems OK to me - you can say:

if ( hp.empty() ) {
   // populate map
}

I would also consider making it a map of values rather than pointers - I don't see you need dynamic allocation here:

 std::map <std::string, HistoParameter> hp;

then:

 hp["ph_pt"] = HistoParameter(100,0,22000);

Note you don't need the explicit std::string conversion. Or better still:

 hp.insert( std::make_pair( "ph_pt", HistoParameter(100,0,22000 )));

Answer 3

The first solution produces a lot of garbage. Why don't you return the class by value? It's quite lightweight, and you wouln't have to dynamically allocate it.

HistoParameter variable_to_parameter(char* var_name)
{
  static std::map<const std::string, HistoParameter> hp;
  if ( hp.empty() )
  {
    hp.insert( std::make_pair( "ph_pt", HistoParameter(100,0,22000) ) );
    hp.insert( std::make_pair( "ph_eta", HistoParameter(100,-3,3) ) );
  //...
  }
  return hp[var_name];
}

If the class returned gets larger, and you want a power-tool, then try out boost::flyweight.

If you don't want to pass back a big structure, you can do:

HistoParameter& variable_to_parameter(char* var_name)
{
  // same code
}

... and even throw in a const if you want it immutable.

Edit: added make_pair, as suggested by Niel.

Answer 4

I'd have a std::map< std::string, HistoParameter *> member and do

InitializeHistoParameter() 
{
   myMap["ph_pt"] = new ...
   myMap["ph_eta"] = new ...
}

And then

HistoParameter* variable_to_parameter(char* var_name) 
{
    return myMap[var_name];
}

Answer 5

either way, you are creating memory leak. each time the = operator is called, for example:

hp[std::string("ph_pt")] = new HistoParameter(100,0,22000);

you are creating a new HistoParameter object and pairing the key "ph" with this most recent object, leaving the previous one dangling. If creating a new object each time is your actual intent, you probably need to call

delete hp[std::string("ph_pt")]; 

before the new operation.

My suggestion is to avoid raw new operations as much as possible and resort to smart pointers such as boost::share_ptr for object life time management.