5
\$\begingroup\$

I'm writing a small application to score matches of a game. (There are two "teams" on each "alliance", and two alliances play each other in a match.) I have a "logic" class that handles the values of different scoring elements for each team, and calculates that team's score.

Instead of writing getters and setters for each member of the "logic" class, the UI class is a friend of the logic class and can access the private "scoring element" members of that class. Whenever a user enters something into the UI, the UI sets the "scoring element" member of that class--but it will pick a different "alliance" and "team" depending on which control was changed. This leads to a lot of code duplication that I want to avoid.

Here is an example:

void BlockParty::on_comboBox_red_A_currentIndexChanged(int index)
{
    switch (index) {
        case 0 :
            score_red->ramp_position_A = BlockPartyLogic::RAMP_OFF;
            break;
        case 1 :
            score_red->ramp_position_A = BlockPartyLogic::RAMP_PARTIAL;
            break;
        case 2 :
            score_red->ramp_position_A = BlockPartyLogic::RAMP_COMPLETE;
            break;
    }
    score_red->update_internals();
}

(ramp_position_A is an example of a "scoring element".) The above function needs to be repeated three more times, as on_comboBox_red_B_currentIndexChanged, on_comboBox_blue_A, and on_comboBox_blue_B.

Is there a good way to consolidate the logic into a single function that can call each slot with different parameters?

For example, something like the following:

void BlockParty::on_comboBox_currentIndexChanged(int index, Score* score)
{
    switch (index) {
        case 0 :
            score->ramp_position_A = BlockPartyLogic::RAMP_OFF;
            break;
        case 1 :
            score->ramp_position_A = BlockPartyLogic::RAMP_PARTIAL;
            break;
        case 2 :
            score->ramp_position_A = BlockPartyLogic::RAMP_COMPLETE;
            break;
    }
    score->update_internals();
}

I could make a giant switch statement to take care of the logic, but that doesn't actually address the problem.

\$\endgroup\$
4
\$\begingroup\$

You could put the statuses into a constant array and use the index variable to fetch them out:

void BlockParty::on_comboBox_currentIndexChanged(int index, Score* score)
{
  static const BlockPartyLogic value_table[] =
  {
    BlockPartyLogic::RAMP_OFF,      // Index 0
    BlockPartyLogic::RAMP_PARTIAL,  // Index 1
    BlockPartyLogic::RAMP_COMPLETE, // Index 2
  };
  if (index < sizeof(value_table) / sizeof(value_table[0]))
  {
    score->ramp_position_A = value_table[index];
  }
    score->update_internals();
}

If you want to select functionality based on an index you could use a static const array of function pointers or function objects.

\$\endgroup\$
  • \$\begingroup\$ Or std::vector (assuming no C++11 here), also allowing the first if to just use size(). \$\endgroup\$ – Jamal Jul 11 '14 at 18:25
  • \$\begingroup\$ The problem is that an std::vector needs to be initialized at runtime. The array is constant data and can be stored in read only memory and directly accessed. No third-party structures need initializing. \$\endgroup\$ – Thomas Matthews Jul 11 '14 at 18:27
  • \$\begingroup\$ That is true, and C++11 would make it nicer with std::array. Still, this looks better than the switch. \$\endgroup\$ – Jamal Jul 11 '14 at 18:29
3
\$\begingroup\$

It's hard to say from such a small view into your code, but generally, yes, you'll want to reduce the duplication in C++ code.

In particular in this case, if you have multiple teams which all have common logic, then that's probably best expressed using an array of identical classes.

class Team { ... };
std::vector<Team> team;
team.emplace_back(Team("red"));
team.emplace_back(Team("blue"));
team.emplace_back(Team("green"));

If you have a number of teams which all have almost the same logic, then there may be a clue that you'd benefit from subclasses.

class Baseteam { ... };
class RedTeam : public Baseteam { ... };
class BlueTeam : public Baseteam { ... };
class GreenTeam : public Baseteam { ... };
std::vector<Baseteam> team;
team.emplace_back(RedTeam());
team.emplace_back(BlueTeam());
team.emplace_back(GreenTeam());

If your intent instead is to reduce the case logic, you could either use an array (with bounds checking, of course) to select logic, or you could use polymorphism. For the former, to rewrite your existing routine, it might be done something like this:

void BlockParty::on_comboBox_currentIndexChanged(int index, Score* score)
{
    static constexpr RAMPVALS = 3;
    static const auto ramp[RAMPVALS] { 
        BlockPartyLogic::RAMP_OFF, 
        BlockPartyLogic::RAMP_PARTIAL, 
        BlockPartyLogic::RAMP_COMPLETE
    };
    if (index < 0 || index > RAMPVALS)
        index = 0;
    score->ramp_position_A = ramp[index];
    score->update_internals();
}

The problem with this is that it brings BlockPartyLogic items into BlockParty. Better would be to push that into BlockPartyLogic instead:

void BlockParty::on_comboBox_currentIndexChanged(int index, Score* score)
{
    score->ramp_position_A = BlockPartyLogic::RampVal(index);
    score->update_internals();
}
\$\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.