Design of Experiments data structure with C++17

Question

I've defined a data structure which represents a doe_model, providing two maps with a configuration_id as key and a configuration_model as value. The configuration_model is an unordered_map which has a knob_name as key and a knob_value as value. There are things I don't like very much about this but I can't get a grasp on how I should change them. For instance: using the next and get_next() trick I'm forced to add a copy constructor and an assignment operator in order to get rid of the undefined behavior I would get by shallow copying the next iterator member. But I don't know if there's a better way to achieve what I need, which is: since I have a thread pool which concurrently access the doe data structure, I have to provide them the next configuration to explore in a smart way. So if I'm reaching the end of the configurations (and there are still some to use) I have to return them from the begin, until I've exhausted all of them.

Here's the code:

using configuration_model = std::unordered_map<std::string, std::string>;

struct doe_model {

  doe_model() {}
  doe_model(const doe_model& doe)
  {
    required_explorations = doe.required_explorations;
    number_of_explorations = doe.number_of_explorations;
    next = required_explorations.end();
  }

  inline doe_model& operator=(const doe_model& ldoe)
  {
    required_explorations = ldoe.required_explorations;
    number_of_explorations = ldoe.number_of_explorations;
    next = required_explorations.end();
    return *this;
  }

  inline bool add_config(const std::string &config_id,
                         const configuration_model &config,
                         const int required_number_of_observations) {
    bool assignment_took_place =
        !required_explorations.insert_or_assign(config_id, config).second ||
        !number_of_explorations
             .insert_or_assign(config_id, required_number_of_observations)
             .second;
    next = required_explorations.end();
    return assignment_took_place;
  }

  inline void update_config(const std::string &config_id) {
    number_of_explorations.at(config_id)--;

    // remove the configuration in case we exausted all the explorations
    if (number_of_explorations.at(config_id) <= 0)
      remove_config(config_id);
  }

  inline void remove_config(const std::string &config_id) {
    required_explorations.erase(config_id);
    number_of_explorations.erase(config_id);
  }

  // this method returns the next configuration to explore
  // NOTE: the caller MUST check the pointer first.
  inline std::map<std::string, configuration_model>::iterator get_next() {
    // we may have an empty map or one with only a single configuration left
    if (required_explorations.empty() || next == required_explorations.end()) {
      next = required_explorations.begin();
      return next;
    }

    next++;

    return next;
  }

  // key is the configuration_id
  std::map<std::string, configuration_model> required_explorations;
  std::map<std::string, configuration_model>::iterator next;
  std::map<std::string, int> number_of_explorations;
};

Answer 1

Because you're defining your member functions within the class declaration, you don't need to use the inline keyword. All member functions declared in the class declaration are implicitly inline (including the default and copy constructors) are inline.

In your copy constructor, you can make use of member initializers to construct the copied maps directly, rather than default construct and assign:

doe_model(const doe_model& doe): required_explorations(doe.required_explorations),
    next(required_explorations.end()), number_of_explorations(doe.number_of_explorations)
}

There are a few places where you're using two statements but they can be combined.

In update_config, you can decrement in the test:

if (--number_of_explorations.at(config_id) <= 0)

In get_next, you can combine the iterator increment and return statement:

return ++next;

This also uses the preincrement on the iterator, as it avoids creating a copy of the original iterator that is returned by the postincrement version, then immediately discarded.

In add_config, you have the potential of adding a config with a higher number of required explorations than are provided if that config already exists (if required_explorations is assigned to, no change is made to number_of_explorations). This may or may not be a problem.

You should consider adding a move constructor and a move assignment operator, which can avoid creating copies.