Clean up Knight Movement method in a chess project with duplicate code

Question

I am writing a chess project using C++ and SDL. In this method I calculate all available moves a Knight can make and I want to refactor it. I basically move the knight Up/Down 2 squares and then Left/Right 1 square and then do the opposite. As you can see in the 2 for loops the difference is the way tempPosition.x and tempPosition.y are incremented and then the 2nd loop is the same as the first,but swapped (see comments)

std::vector<Position> available;
    Position tempPosition;

    int directions_double[2] = {-2, 2};
    int directions_single[2] = {-1, 1};
  
    for(int double_step : directions_double) {
        tempPosition = position_;
        tempPosition.x += double_step;        // x+=doublestep
        Position temp2_position = tempPosition;
        for(int single : directions_single) {
            tempPosition = temp2_position;
            tempPosition.y += single;          //y+=single step
            if(InBoard(tempPosition))
                available.push_back(tempPosition);
            else
                break;
        }
    }
    for(int double_step : directions_double) {
        tempPosition = position_;
        tempPosition.y += double_step;        //y += doublestep
        Position temp2_position = tempPosition;
        for(int single : directions_single) {
            tempPosition = temp2_position;
            tempPosition.x+=single;           //x+= single step
            if(InBoard(tempPosition))
                available.push_back(tempPosition);
            else
                break;
        }
    }
    return available;
}

The difference is very small. I thought of implementing a flag technique, but it didn't seem 'clean' enough. Any thoughts? Thank you.

G. Sliepen · Accepted Answer · 2021-09-19 13:08:53Z

Use `Position`s for steps as well

You can avoid the duplication by using Positions to represent the steps as well:

Position directions_double[] = {{-2, 0}, {2, 0}, {0, -2}, {0, 2}};
...
for (auto double_step: directions_double) {
    ...
    tempPosition.x += double_step.x;
    tempPosition.y += double_step.y;
    ...
}

If the type Position has overloads for operator+, then you could simplify the additions to:

tempPosition += double_step;

For the inner loop, observe that the single steps are just the same as the double steps, but x and y swapped and their values halved:

for (auto& double_step: directions_double) {
    ...
    Position single_steps[] = {
        {double_step.y / 2, double_step.x / 2},
        {-double_step.y / 2, -double_step.x / 2},
    };
    for (auto& single: single_steps) {
        ...
    }
}

Alternatively, create one array of steps representing all possible knight moves:

Position knight_moves[] = {
    {-2, -1}, {-2,  1},
    {-1, -2}, {-1,  2},
    ...
};

With this, the loop becomes even simpler.

Use consistent naming

You use double_step as the value for the outer loop, but single for the inner loop. Of course you can't just use double as the name of a variable, but you could make it more consistent by writing single_step.

Incorrect `break` statement?

It looks to me like you are trying to check for valid knight moves on a chess board. If so, I think the break statement in the inner loops is incorrect.

Avoid unnecessary temporary variables

The temporary variables currently make the code hard to follow. It would be nice to remove as many of them as possible, and/or give them better names than "temporaryThing". Ideally, the code would look like:

Position knight_moves[] = {...};

for (auto& step: knight_moves) {
    auto new_position = position_ + step; 
    if (InBoard(new_position)) {
        available.push_back(new_position);
    }
}

This assumes you can add two Positions together. If not, you can probably write this instead:

Position new_position = {position_.x + step.x, position_.y + step.y};

While new_position is still a temporary variable, it's the only one, and it has a slightly better name.

@Sliepen Thank you for your input.Best solution I can see is creating one array of Positions and overloading operator+.You are correct,this is a chess project me and a friend are working on and this is indeed the Knight class. I will definetely take your naming input as well since we try to keep things as clean as possible. Any other suggestions you might have before i accept the answer? — laegirl
– laegirl, Commented Sep 19, 2021 at 14:42
You could make the array static constexpr, although a decent compiler will probably generate equally efficient code either way. That's all the suggestions I have. — G. Sliepen
– G. Sliepen, Commented Sep 19, 2021 at 14:56

mdfst13 · Accepted Answer · 2021-09-20 08:14:32Z

0

Posting the final code using @G. Sliepen input. It turned pretty clean, also used this logic to refactor the rest of the Chess Pieces and the project shrank in lines.


//Returns an std::vector<Position> with all available positions a Knight piece
//can attack/move
std::vector<Position>
Knight::AvailableMoves(const std::vector<Material*>& other) const {
  static const int kMovableDirections = 8;
  static const Position moves[kMovableDirections] = {
    {2, 1}, {2, -1}, {-2, 1}, {-2, -1}, {1, 2}, {-1, 2}, {1, -2}, {-1, -2}
  };
    std::vector<Position> available;
  for (Position move : moves) {
    Position possible_position = position_ + move;
    if (PositionValid(possible_position, other, color_)) { //Position is valid if in 
      available.push_back(possible_position);              //Board and not occupied 
    }                                                      //by ally piece
  }
  return available;
}

edited Sep 20, 2021 at 8:14

mdfst13

22.4k6 gold badges34 silver badges70 bronze badges

answered Sep 20, 2021 at 8:02

laegirl

1433 bronze badges

\$\begingroup\$ Looks great already. But if you want the new code to be reviewed, I recommend posting it as a new question on this site! \$\endgroup\$

G. Sliepen
– G. Sliepen

2021-09-20 08:56:22 +00:00
Commented Sep 20, 2021 at 8:56
\$\begingroup\$ No thank you man,we are looking good for now. We will work on the Board today. If it turns messy I will ask for opinions again :D \$\endgroup\$

laegirl
– laegirl

2021-09-20 08:59:29 +00:00
Commented Sep 20, 2021 at 8:59

Add a comment |

JDługosz · Accepted Answer · 2021-09-20 14:46:43Z

I've done similar things by changing the members (x and y in your case; left and right for doing mirror-image on a binary tree without duplicating the code) to be an array of 2 instead. Then you can have this_way and that_way set to 0 and 1 in either way, to have the same code work for the transposed case.

tempPosition.x becomes tempPosition.xy[this_way] and tempPosition.y becomes tempPosition.xy[that_way]. The subroutine takes this_way as an argument (0 or 1) and generates that_way as a local variable 1-this_way.

Now the two nearly identical blocks of code that differ only in switching the use of x and y are one function with an additional argument.

Stack Exchange Network

Clean up Knight Movement method in a chess project with duplicate code

3 Answers 3

Use `Position`s for steps as well

Use consistent naming

Incorrect `break` statement?

Avoid unnecessary temporary variables

You must log in to answer this question.

Hot Network Questions

Clean up Knight Movement method in a chess project with duplicate code

3 Answers 3

Use Positions for steps as well

Use consistent naming

Incorrect break statement?

Avoid unnecessary temporary variables

You must log in to answer this question.

Related

Hot Network Questions

Use `Position`s for steps as well

Incorrect `break` statement?