Processing player's move in an ASCII rogue-like game

Question

I'm making an ASCII roguelike game and while writing the code for processing the player's move I can't decide between these two options. I have defined a class named point to handle positions in the game, like this. I would also like to know if this is a good idea or if I should use a structure instead.

Should I pass an object as a parameter to a function or create it inside?

class point {
public:
 point();
 point(int x, int y);
 int getX() const;
 int getY() const;;
 void setX(int x);
 void setY(int y);
private:
 int x;
 int y;
};

OPTION 1

I define the target point object before calling the function.

point playerPos = player.get_position();
point moveTilePos;
switch (input) {
 case MOVE_UP:
 moveTilePos.setX(playerPos.getX());
 moveTilePos.setY(playerPos.getY()-1);
 processPlayerMove(player, moveTilePos);

which is defined as follows:

void Level::processPlayerMove(Player &player, point target) {
 char moveTileSymbol;
 moveTileSymbol = getTile(target);
 switch(moveTileSymbol){
 case WALL_SYMBOL:
 break;
 case GROUND_SYMBOL:
 setTile(player.get_position(), GROUND_SYMBOL);
 player.set_position(target);
 setTile(target, PLAYER_SYMBOL);
 break;
 }
}

I think this is more readable overall but requires a setup that maybe should be part of the function.

Option 2

Everything is handled by the function:

point playerPos = player.get_position();
switch (input) {
 case MOVE_UP:
 processPlayerMove2(player, playerPos.getX(), playerPos.getY() -1)

Which is defined like this:

void Level::processPlayerMove2(Player &player, int targetX, int targetY) {
 char moveTileSymbol;
 point target;
 target.setX(targetX);
 target.setY(targetY);
 moveTileSymbol = getTile(target);
 switch(moveTileSymbol){
 case WALL_SYMBOL:
 break;
 case GROUND_SYMBOL:
 setTile(player.get_position(), GROUND_SYMBOL);
 player.set_position(target);
 setTile(target, PLAYER_SYMBOL);
 break;
 }
}

This case is more succinct, as the function handles everything.

Answer 1

point playerPos = player.get_position();
point moveTilePos;
moveTilePos.setX(playerPos.getX());
moveTilePos.setY(playerPos.getY()-1);
processPlayerMove(player, moveTilePos);

Well, of course you shouldn't do that. Declaring an "uninitialized" variable to be initialized later via mutation is one of the cardinal sins of modern C++ programming. One reasonable way to write this code would be

point playerPos = player.get_position();
processPlayerMove(
 Point(playerPos.getX(), playerPos.getY() - 1)
);

But if you find yourself writing Point(p.getX(), p.getY()-1) often enough, then you should factor it out into a function:

point playerPos = player.get_position();
processPlayerMove(playerPos.upward(1));

And then you can eliminate that pesky helper variable:

processPlayerMove(player.get_position().upward(1));

There, that looks clean enough!

---

Next, look at your switch statement and refactor it to be clearer.

switch (input) {
 case MOVE_UP:
 processPlayerMove(player.get_position().upward(1));
 break;
 case MOVE_DOWN:
 processPlayerMove(player.get_position().downward(1));
 break;
 case MOVE_LEFT:
 processPlayerMove(player.get_position().leftward(1));
 break;
 case MOVE_RIGHT:
 processPlayerMove(player.get_position().rightward(1));
 break;
}

Can you think of a similar way to simplify this code? What would it look like?

• Santiago: Thanks! I don't know how to simplify that switch statement even more. I was thinking maybe putting it all in another function, passing a pointer to the movement function (upward, downward, etc) and the input from the user. But that doesn't make much sense... I will think about it. Thanks a lot again!
• Santiago: Shouldn't I pass player to the process function? as I want it to change the positions of the sprites in the level. Also, i've implemented two new methods in the Point class: point verticalShift (int offset); and point horizontalShift (int offset); to handle the shifting
• Quuxplusone: "Shouldn't I pass player to the process function?" Yes, probably. You might even try to reuse the same movement code for the player and for monsters/creatures, since both pieces of code need to answer the same question: "Can actor X move from Y to Z under its own power?" (For NPCs, you also need to decide whether X wants to move that direction.)
• Quuxplusone: Refactoring .upward(1), .downward(1) into .verticalShift(-1), .verticalShift(1) is probably a good start. How about refactoring .verticalShift(1), .horizontalShift(1) into .plus(0,1), .plus(1,0)?

Answer 2

Use option 3: Create a movevector-object when calling the function, and let the function do everything.

---

Is there any reason you give the position to move to, even before doing any validation, instead of a direction-vector?
That leads to code-duplication.

struct movevector {
 int x, y;
};
movevector operator-(movevector v) noexcept { return {-v.x, -v.y}; }
movevector move_x(int x) noexcept { return {x, 0}; }
movevector move_y(int y) noexcept { return {0, y}; }
switch(input) {
case MOVE_UP: processPlayerMove(player, move_y(-1)); break;
...

Though only add move_x and move_y if you use them often enough.

You should also explore whether you could use the same function to process movement for all entities, or players are too special.

Also, I really wouldn't do faux-encapsulation with point, there's no point to it.
Remove the ctors, setters, and getters, add some operators to add / subtract movevectors, and you have a much more useful abstraction which doesn't encourage bloated code.

struct point {
 int x, y;
};
point operator+(point p, movevector d) noexcept { return {p.x + d.x, p.y + d.y}; }
point operator-(point p, movevector d) noexcept { return p + -d; }
point& operator+=(point& p, movevector d) noexcept { return p = p + d; }
point& operator-=(point& p, movevector d) noexcept { return p = p - d; }

• Santiago: Is it a good practice to define both point and moveVector in the same file? As they are closely related. Also, is it a good idea to have a Movement class (or something with a better name), with both structures and all methods and operators?
• Santiago: Also, i'm getting this error error: invalid use of 'this' in non-member function point& operator-=(point& p, movevector d) noexcept { return *this = p - d; }
• Deduplicator: @Santiago Fixed the error. That comes from moving code around and not verifying all parts are fixed. A header should provide all the related parts of an interface, whether it consists of one or a hundred symbols, be they classes, aliases, variables, functions or whatever.

Answer 3

These are both reasonable ways to do what you're trying to do, but I think you've missed an opportunity to make this even easier. Instead of having a class that only has a setX()/setY(), why not make some operators for doing math on your points? I would start by removing the default constructor via:

 point() = delete;

Then I would add something like:

point operator+(const point& rhs);
point& operator+=(const point& rhs);

I would stop thinking about separate x and y coordinates as much as possible and start thinking about points, locations, velocities, etc. This would allow you to write your code as:

point playerPos = player.get_position();
switch (input) {
 case MOVE_UP:
 processPlayerMove(player, playerPos + point(0, -1));

I strongly dislike option 2 because it treats a single entity, the point, as 2 different values, when they really are a single thing.