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

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.

# Use Positions 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? 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. Sep 19, 2021 at 14:56
• Thank you sir,have a nice day Sep 19, 2021 at 15:00

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

• Looks great already. But if you want the new code to be reviewed, I recommend posting it as a new question on this site! Sep 20, 2021 at 8:56
• 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 Sep 20, 2021 at 8:59

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.