6
\$\begingroup\$

I am making a Ping Pong game using Html5 Canvas. I am concerned about the movement(_b) method. This method should move the computer paddle (up and down) vertically and follow the ball. Some sort of simple AI. Everything works like it should but there is a lot of repetition of if else blocks that might be handled differently. Like shorter code or at least a bit more elegant.

The _b in the movement(_b) is parameter for the ball that will be passed as an argument.

The keyword this refers to the computer paddle

The code below might give you an idea of what I am talking about.

class Paddle {
    constructor(x, y, width, height, color) {
        this.x = x
        this.y = y
        this.width = width
        this.height = height
        this.color = color
    }
    // collision  logic here...
}


class Ball {
    constructor(x, y, x_velocity, y_velocity, radius, color) {
        this.x = x;
        this.y = y;
        this.velocity = {
            x: x_velocity,
            y: y_velocity
        }
        this.radius = radius;
        this.color = color;
        this.isBouncingAllowed = true;
    }
    // movement logic here...
}

 class ComputerPaddle extends Paddle {
    constructor(...args) {
        super(...args)
    }

    movement(_b) {
        let centerOfPaddle = this.y + this.height / 2;

        if (centerOfPaddle < _b.y - 80) {
            this.y += 4
        }
        else if (centerOfPaddle > _b.y + 80) {
            this.y -= 4
        }
        else if (centerOfPaddle < _b.y - 60) {
            this.y += 2
        }
        else if (centerOfPaddle > _b.y + 60) {
            this.y -= 2
        }
        else if (centerOfPaddle < _b.y - 40) {
            this.y += 1
        }
        else if (centerOfPaddle > _b.y + 40) {
            this.y -= 1
        }
        else if (centerOfPaddle < _b.y - 20) {
            this.y += 0.5
        }
        else if (centerOfPaddle > _b.y + 20) {
            this.y -= 0.5
        }
    }
    update(_ball_) {
        this.movement(_ball_)
        this.ballInteraction(_ball_)
        this.draw()
    }
}

let player = new PlayerPaddle(20, canvas.height / 2 - sizes.paddle.height / 2, sizes.paddle.width, sizes.paddle.height, 'red')
let computer = new ComputerPaddle(canvas.width - (20 + sizes.paddle.width), canvas.height / 2 - sizes.paddle.height / 2, sizes.paddle.width, sizes.paddle.height, 'cyan')
let ball = new Ball(250, 350, 5, 0, sizes.ball.radius, 'green')


function animate() {
    requestAnimationFrame(animate);
    ctx.clearRect(0, 0, canvas.width, canvas.height)

    ball.update()
    player.update(ball)
    computer.update(ball)
}
animate();

Any suggestions will be much appreciated.

EDIT: I did add the Ball and Paddle constructors. The methods in the classes containing the movement and collision detection logic were not included because that code is over 100 lines long and probably not relevant for my question.

\$\endgroup\$
12
  • 3
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Nov 20 '20 at 8:39
  • 1
    \$\begingroup\$ Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Please follow the tour, and read "What topics can I ask about here?", "How do I ask a good question?" and "What types of questions should I avoid asking?". \$\endgroup\$
    – BCdotWEB
    Nov 20 '20 at 8:39
  • 2
    \$\begingroup\$ This is not Stack Overflow, and its mods are not this SE's mods. We have own rules, please adhere to them. \$\endgroup\$
    – BCdotWEB
    Nov 20 '20 at 9:28
  • 1
    \$\begingroup\$ Shame you shut this down as I had a much better answer than what you have. \$\endgroup\$
    – Blindman67
    Nov 20 '20 at 13:20
  • 3
    \$\begingroup\$ It's not too late to fix this question to get it reopened. Just tell us more about what this code is designed to accomplish ("AI to control a Pong paddle", maybe?) and retitle the question accordingly. Add some detail about what this and _b are supposed to represent. \$\endgroup\$ Nov 21 '20 at 8:54
3
\$\begingroup\$

Pong

This code looks like part of a pong like game logic.

With that in mind we can make some changes to the naming

Replacing the function movement with movePaddle and the argument to ball

Using a lookup table

Then in 3 steps

  • Find the vertical distance to the ball from the paddle center.

  • Convert that distance to an index used to lookup speeds.

  • Use the sign of the distance to convert the speed to the correct sign.

Example

const SPEEDS = [0, 0.5, 1, 2, 4]; 

movePaddle(ball) {
    const distance = ball.y - (this.y + this.height / 2);
    const idx = Math.min(Math.abs(distance / 20) | 0, SPEEDS.length - 1);
    this.y += SPEEDS[idx] * Math.sign(distance);
}

Smooth change

However I am guessing that the speeds are somewhat arbitrary and thus a functional approximation will do just as well. That the speed is a function of distance. A hermite curve looks like a close approximation.

The result is a smooth change in speed as he ball gets closer , but depending on how the ball moves this will need to be tuned to get the correct look and feel.

const MAX_SPEED = 4;
const MIN_SPEED_CUTOFF = 0.5;
const DISTANCE_RANGE = 80;


movePaddle(ball) {
    const dist = (ball.y - (this.y + this.height / 2)) / DISTANCE_RANGE;
    const uDist = Math.abs(dist < -1 ? -1 : dist > 1 ? 1 : dist);
    const y = (uDist * uDist * (3 - 2 * uDist)) * MAX_SPEED; // hermite curve 
    this.y += (y < MIN_SPEED_CUTOFF ? 0 : y) * Math.sign(dist);
}

Further improvements

One can go further and use the balls distance from the paddle (horizontal) to calculate how long until the ball reaches the edge. This time can then be used to scale the speed so that the paddle can be at an exact position when the ball arrives.

Also using the balls horizontal speed to workout if it is coming or going. Thus the paddle need move only when it needs to rather than track the ball all the time.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks a lot. I really appreciate your effort. It helped me a lot. \$\endgroup\$ Nov 22 '20 at 4:17
  • \$\begingroup\$ A honest question now. Do I change this to the accepted answer. I am asking because I don't want to be rude or unthankful to anyone and I am not sure what are the policies about these kind of things here. \$\endgroup\$ Nov 22 '20 at 4:24
  • \$\begingroup\$ @HappyCoconut There is no policy in regard to accepting answers. Accept the answer that you feel most helped you. \$\endgroup\$
    – Blindman67
    Nov 22 '20 at 5:19
8
\$\begingroup\$

Using a mechanical refactoring approach, you could create an array of objects to encapsulate the offsets and values associated with each offset:

movement(_b) {
  let centerOfPaddle = this.y + this.height / 2;

  let offsets = [
    { offset: 80, value: 4 },
    { offset: 60, value: 2 },
    { offset: 40, value: 1 },
    { offset: 20, value: 0.5 },
  ];

  for (const { offset, value } of offsets) {
    if (centerOfPaddle < _b.y - offset) {
      this.y += value;
      break;
    } else if (centerOfPaddle > _b.y + offset) {
      this.y -= value;
      break;
    }
  }
}

Going further, you could do a more conceptual refactoring and figure out the math behind your logic. For example:

movement(_b) {
  let centerOfPaddle = this.y + this.height / 2;

  for (let i = 80, j = 4; i >= 20; i -= 20, j /= 2) {
    if (centerOfPaddle < _b.y - i) {
      this.y += j;
      break;
    } else if (centerOfPaddle > _b.y + i) {
      this.y -= j;
      break;
    }
  }
}

You could give those loop variables more appropriate names like offset and value.

\$\endgroup\$
6
  • \$\begingroup\$ Hey thanks a lot for the help. The first example doesn't seems to work properly. No errors but it always gets this 0.5 speed. The second example works great. \$\endgroup\$ Nov 20 '20 at 1:59
  • \$\begingroup\$ Ah, that is because Object.entries does not guarantee the order of properties. You need to use an array instead. Let me add an example for that. In any case, the last example is probably best. \$\endgroup\$
    – elclanrs
    Nov 20 '20 at 2:05
  • \$\begingroup\$ I updated the first example to use an array instead. Now it should work as expected. \$\endgroup\$
    – elclanrs
    Nov 20 '20 at 2:07
  • \$\begingroup\$ give me a sec to try it \$\endgroup\$ Nov 20 '20 at 2:07
  • 1
    \$\begingroup\$ It works great. Thanks dude for the help. Have a nice day! \$\endgroup\$ Nov 20 '20 at 2:08
4
\$\begingroup\$

To cut down on code you can pull _b.y to the left-hand side of the inequality and chain the ternary operator like so:

function movement(_b) {
  const centerOfPaddle = this.y + this.height / 2;
  const dist = centerOfPaddle - _b.y

  const delta = (
    dist < -80 ?  4 :
    dist < -60 ?  2 :
    dist < -40 ?  1 :
    dist < -20 ?  .5 :
    dist >  80 ? -4 :
    dist >  60 ? -2 :
    dist >  40 ? -1 :
    dist >  20 ? -.5 :
    0
  )

  this.y += delta
}

The other option would be exploiting the symmetry of your breakpoints with something like

function movement(_b) {
    breakPoints = [
        { dist: 80, delta: 4 },
        { dist: 60, delta: 2 },
        { dist: 40, delta: 1 },
        { dist: 20, delta: .5 },
    ]

    const centerOfPaddle = this.y + this.height / 2;
    const distFromCenter = centerOfPaddle - _b.y

    b = breakPoints.find(e => 
        ( distFromCenter < -e.dist ) ||
        ( distFromCenter >  e.dist )
    )
    
    const delta = (
        ( distFromCenter < -b.dist ) ? b.delta :
        ( distFromCenter >  b.dist ) ? -b.delta :
        0
    )

    this.y += delta
}

Maybe overkill for your problem but it lets you update the breakpoints and values you're evaluating a bit more easily if you need to change them

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks for the feedback. The first example works great. The second one doesn't. Gives me error on this piece of code "(distFromCenter < -b.dist) ? b.delta :". The error is "Uncaught TypeError: Cannot read property 'dist' of undefined". Also i had to put let before breakPoints and b because this is class method and it didn't run without the let. \$\endgroup\$ Nov 20 '20 at 1:56
  • \$\begingroup\$ I didn't really test the second one beyond a couple values so not surprised. I'm guessing breakPoints.find() returns undefined for some range of values so there should be something that catches b === undefined and sets delta=0 in that case before any reference to b.dist if that makes sense. My goal was more showing a potential way to exploit the symmetry of your problem than provide a a well tested solution. Hope you can see the overall goal! \$\endgroup\$
    – Coupcoup
    Nov 20 '20 at 2:07
  • \$\begingroup\$ Thanks for your answer. Much appreciated. \$\endgroup\$ Nov 20 '20 at 2:10
  • 1
    \$\begingroup\$ Made some changes to the code " const delta = ( (b === undefined) ? 0 : (distFromCenter < -b.dist) ? b.delta : (distFromCenter > b.dist) ? -b.delta : 0 )" and it works now. If some noob like me in the future needs it. \$\endgroup\$ Nov 20 '20 at 2:23
4
\$\begingroup\$

First, this kind of expressions:

centerOfPaddle < _b.y - 80

can be rewritten as

centerOfPaddle - _b.y < -80  

So let's put it in a variable:

let offset = centerOfPaddle - _b.y

Next, you have a mirror symmetry so let's not duplicate it for both sides, rather establish first which side it is, for example:

let invertFactor = 1
if (offset < 0){ invertFactor = -1 }

Now we can write a switch for only one of each of those thresholds and the invertFactor takes care of the left/right side:

switch(Math.floor(Math.abs(offset / 20))){
   case 4:
      this.y -= 4 * invertFactor
      break
   case 3:
      this.y -= 2 * invertFactor
      break
   case 2:
      this.y -= 1 * invertFactor
      break
   case 1:
      this.y -= 0.5 * invertFactor
      break
}
\$\endgroup\$
2
  • \$\begingroup\$ The case numbers 4, 3, 2, 1 and the factors 4, 2, 1, 0.5 are closely related. factor = 2 ^ (case_number - 2) which translates to const case_number = Math.floor(Math.abs(offset / 20)); this.y -= Math.pow(2, case_number - 2) * invertFactor;. That looks even simpler than the math version by @elclanrs since it doesn't have a loop. \$\endgroup\$
    – nwp
    Nov 20 '20 at 12:36
  • \$\begingroup\$ One could also argue that const invertFactor = offset < 0 ? -1 : 1; is better. \$\endgroup\$
    – nwp
    Nov 20 '20 at 12:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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