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?

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

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.