3
\$\begingroup\$

I want to map values of a enum to some other values. For instance, here I map Color to Group:

enum class Color {
  Red, Green, Blue, Cyan, Magenta, Yellow, White, Black
};
enum class Group {
  Primary, Secondary, Neutral
};

One particular interesting case is to map enum to a string (serialize), but I'm interested in the general case. I came out with three possibilities of doing this:

Group GetColorGroupMap(Color color)
{
  static const std::map<Color, Group> color2group = {
    { Color::Red, Group::Primary },
    { Color::Green, Group::Primary },
    { Color::Blue, Group::Primary },
    { Color::Cyan, Group::Secondary },
    { Color::Magenta, Group::Secondary },
    { Color::Yellow, Group::Secondary },
    { Color::White, Group::Neutral },
    { Color::Black, Group::Neutral }
  };

  // Shall I check the iterator here?
  return color2group.find(color)->second;
}

Group GetColorGroupArr(Color color)
{
  static const Group color2group[] = {
    Group::Primary,    // Red
    Group::Primary,    // Green
    Group::Primary,    // Blue
    Group::Secondary,  // Cyan
    Group::Secondary,  // Magenta
    Group::Secondary,  // Yellow
    Group::Neutral,    // White
    Group::Neutral     // Black
  };

  // Shall I check the index here?
  return color2group[size_t(color)];
}


Group GetColorGroupSwitch(Color color)
{
  switch (color)
  {
  case Color::Red: return Group::Primary;
  case Color::Green: return Group::Primary;
  case Color::Blue: return Group::Primary;
  case Color::Cyan: return Group::Secondary;
  case Color::Magenta: return Group::Secondary;
  case Color::Yellow: return Group::Secondary;
  case Color::White: return Group::Neutral;
  case Color::Black: return Group::Neutral;
  }

  // Shall I handle this branch here?
  throw std::logic_error("How did we get here?");
}

Questions

  1. What is the best solution among these and why? Maybe there are others?
  2. Shall I do the checks, mentioned in each function?
  3. Is there a way to make these functions robust to refactoring, e.g. when another color is added?

My thoughts

About question 1

  • GetColorGroupMap: potentially the most slow function, since it traverses std::map on each call.
  • GetColorGroupArr: the least obvious structure, since colors are not mentioned explicitly, they are deduced from array indices. Additionally, it won't work with non-sequential enums.
  • GetColorGroupSwitch: a little verbose and smells a little old-school. I do not see any particular problems with it here, though.

About question 2

I think no checks are necessary, since failures are possible only if passed Color is out of specified range, which means, something went wrong before.

About question 3

The only possibility I see is to modify GetColorGroupArr by adding static_assert to check that array size is equal to the number of items in Color. For this, we may add an extra item COUNT to Color. I do not like this approach, since we need to check against this special item in every place enum is used.

See also: https://stackoverflow.com/q/28602114/261217

\$\endgroup\$
  • \$\begingroup\$ May I note an alternative approach would be to define a templated traits<> class defining the mappings at compile time? \$\endgroup\$ – Keith Feb 19 '15 at 3:53
  • \$\begingroup\$ Interesting approach, I did not think about it. However, it may be used also on compile time only. \$\endgroup\$ – Mikhail Feb 19 '15 at 8:31
  • \$\begingroup\$ Yes, it defines a look up at compile time. But if you code it up, note that the run time look up also becomes more robust to change. It can refer to the compile time table so only needs updated when Color is extended, not group. \$\endgroup\$ – Keith Feb 19 '15 at 22:39
  • \$\begingroup\$ I don't get your idea. Could you please post the code as an answer? \$\endgroup\$ – Mikhail Feb 20 '15 at 9:18
1
\$\begingroup\$

Each of these three are obviously valid, but they also have their problems.

Possibility 1:

This, in my opinion, is the best option as it explicitly matches a color with a group. I have no experience with maps in C++, so I don't know how slow it is, but for this small of a data set, I do not think this will be an issue.

Possibility 2:

I do not recommend this option. It does not show any correlation between the two groups, and it will be easy to break by accidentally adding any updates to the wrong location.

Possibility 3:

This is also a valid option, but I would only use it if the color can be used independent of the group and you need to pair/convert them only in certain uses. Also, this code can be cleaned up a little:

switch (color)
{
    case Color::Red:
    case Color::Green:
    case Color::Blue:
        return Group::Primary;
    case Color::Cyan:
    case Color::Magenta:
    case Color::Yellow:
        return Group::Secondary;
    case Color::White:
    case Color::Black:
        return Group::Neutral;
}

Edit:

Oops, I missed the three questions.

  1. I think the top item is the best.
  2. It would not be a bad idea to do the tests, especially if this code will eventually be maintained by others. However, if it is one-time code that will never be touched after it is tested, it may not be necessary.
  3. I do not know of any way to make the tests more robust. However, I think the first and third are the least likely to be messed up when updating because it is immediately obvious what you are returning.
\$\endgroup\$
  • \$\begingroup\$ Thank you. Any thoughts on question 3? \$\endgroup\$ – Mikhail Feb 18 '15 at 15:52
  • \$\begingroup\$ Regarding the speed of maps, std::unordered_map (available since C++11) is the equivalent of a hash map in every other language. \$\endgroup\$ – chris Feb 18 '15 at 17:33
  • \$\begingroup\$ @Mikhail Updated. \$\endgroup\$ – Hosch250 Feb 18 '15 at 20:27
2
\$\begingroup\$

As requested, the templated approach with additional run-tie dispatch:

template <int Color>
struct Traits
{};

template<>
struct Traits<static_cast<int>(Color::Black)>
{
    static const Group group = Group::Neutral;
    static std::string asString() { return "black"; }
};
template<>
struct Traits<static_cast<int>(Color::Red)>
{
    static const Group group = Group::Primary;
    static std::string asString() { return "red"; }
}; 
template<>
struct Traits<static_cast<int>(Color::Green)>
{
    static const Group group = Group::Primary;
    static std::string asString() { return "green"; }
};
template<>
struct Traits<static_cast<int>(Color::Magenta)>
{
    static const Group group = Group::Secondary;
    static std::string asString() { return "magenta"; }
};


Group groupFromColour(Color col)
{
    switch (col)
    {
    case Color::Black:
        return Traits<static_cast<int>(Color::Black)>::group;
        break;
    case Color::Red:
        return Traits<static_cast<int>(Color::Red)>::group;
        break;
    case Color::Green:
        return Traits<static_cast<int>(Color::Green)>::group;
        break;
    // etc.
    }
    // whatever error handler as before.
}

Note how in the switch statement, the Black entry only refers to the Black Traits. You might also use a separate Traits for each mapping, so as you add a need for a new one, existing code is not changed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.