HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Scoring matches of a game - reducing duplicated code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
matchesscoringreducingduplicatedgamecode

Problem

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;
}

Solution

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 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.

Code Snippets

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();
}

Context

StackExchange Code Review Q#56779, answer score: 4

Revisions (0)

No revisions yet.