8
\$\begingroup\$

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.

\$\endgroup\$

3 Answers 3

8
\$\begingroup\$
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?

\$\endgroup\$
4
  • \$\begingroup\$ 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! \$\endgroup\$
    – Santiago
    Aug 27, 2018 at 4:14
  • 1
    \$\begingroup\$ 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 \$\endgroup\$
    – Santiago
    Aug 27, 2018 at 4:26
  • 1
    \$\begingroup\$ "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.) \$\endgroup\$ Aug 27, 2018 at 18:12
  • 1
    \$\begingroup\$ 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)? \$\endgroup\$ Aug 27, 2018 at 18:13
4
\$\begingroup\$

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; }
\$\endgroup\$
3
  • \$\begingroup\$ 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? \$\endgroup\$
    – Santiago
    Aug 27, 2018 at 23:44
  • \$\begingroup\$ 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; } \$\endgroup\$
    – Santiago
    Aug 27, 2018 at 23:59
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Aug 28, 2018 at 1:16
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.