2
\$\begingroup\$

The great feedback I got on my first post on Code Review made me wary of code repetition. This code does what it is supposed to, which is to create restrictions for a sprite's possible position on the screen.

Is this a neat looking solution, or can it be considered code repetition?

public void move(int x, int y) {
    int maxX = screenWidth - image.getWidth()/2;
    int minX = image.getWidth() / 2;
    int maxY = screenHeight - image.getHeight()/2;
    int minY = image.getHeight() / 2;

    if (x > maxX)
        this.x = screenWidth - image.getWidth();
    else if (x < minX)
        this.x = 0;
    else
        this.x = x - image.getWidth() / 2;

    if (y > maxY)
        this.y = screenHeight - image.getHeight();
    else if (y < minY)
        this.y = 0;
    else
        this.y = y - image.getHeight() / 2;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ You certainly can reuse minX and minY more \$\endgroup\$ May 2, 2020 at 9:17

1 Answer 1

2
\$\begingroup\$

It is possible to refactor your code starting from the definition of your variables:

int maxX = screenWidth - image.getWidth()/2;
int minX = image.getWidth() / 2;
int maxY = screenHeight - image.getHeight()/2;
int minY = image.getHeight() / 2;

You are repeating function calls image.getWidth(), image.getHeight() and divisions by 2 in more than one definition, while you could call store functions calls results and division results in other variables like below:

int width = image.getWidth();
int height = image.getHeight();
int minX = width / 2;
int minY = height / 2;
int maxX = screenWidth - minX;
int maxY = screenHeight - minY;

The other code you can simplify is the following:

if (x > maxX)
        this.x = screenWidth - image.getWidth();
    else if (x < minX)
        this.x = 0;
    else
        this.x = x - image.getWidth() / 2;
    // same behaviour of the above code
    if (y > maxY)
        this.y = screenHeight - image.getHeight();
    else if (y < minY)
        this.y = 0;
    else
        this.y = y - image.getHeight() / 2;

It is the same code applied one time to width and other time to height : you could define a helper function to be applied two times like below:

private int getNewCoordinate(int c, int l, int max, int min, int maxl) {
        if (c > max) { return maxl - l; }
        if (c < min) { return 0; }
        return c - l / 2;
}

Now if I haven't made confusion with some variable your original code can be written like this:

public void move(int x, int y) {
    int width = image.getWidth();
    int height = image.getHeight();
    int minX = width / 2;
    int minY = height / 2;
    int maxX = screenWidth - minX;
    int maxY = screenHeight - minY;

    this.x = getNewCoordinate(x, width , maxX, minX, screenWidth);
    this.y = getNewCoordinate(y, height, maxY, minY, screenHeight);   
}

private int getNewCoordinate(int c, int l, int max, int min, int maxl) {
    if (c > max) { return maxl - l; }
    if (c < min) { return 0; }
    return c - l / 2;
}
\$\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.