@@ -0,0 +1,123 @@
let gameState = 'start';

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

There are a lot of hardcoded strings throughout this app:

  • Game states: "start", "play"
  • Keys that players have pressed to move the paddles or start the game: "w", "s","ArrowUp", "ArrowDown", "Enter"
  • And even some hard-coded numbers: 0 and 1 are used to represent the four directions that the ball can move

Strings are great to represent state concepts: they are clear and expressive indicators of what a state change signifies. I would argue that numbers are not as expressive as strings for communicating state changes. The usage of numbers for ball direction is kind of "boolean" in nature: 0 for "up", 1 for "down", but you have to study the code closely to understand what 0 actually means in the context in which it's being used. A string that clearly states "up" or "down" doesn't require close study to understand.

So! We've got some good strategies for representing state change, and some strategies that would benefit from refactoring to strings, but we can do better still. Seeing as the strings are hard-coded, it makes it easier to misspell them, or forget to update all of the instances of a hardcoded string if you decide to refactor the string in the future.

We can make our own home-rolled Typescript-style enums to save these strings in objects. This lets us divide the strings up into categories (states, movement keys, ball directions), and makes them easy and foolproof to update or expand if we need to:

const MovementKeys = {
P1_UP: "w",
P1_DOWN: "s",
P2_UP: "ArrowUp",
P2_DOWN: "ArrowDown",
};

const Keys = {
ENTER: "Enter",
...MovementKeys,
};

const GameStates = {
START: "start",
PLAY: "play",
};

const BallDirections = {
UP: "up",
DOWN: "down",
LEFT: "left",
RIGHT: "right",
};

// usage examples: 
let gameState = GameStates.START;

if (e.key == MovementKeys.P1_UP) {
// move p1 paddle up
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of hardcoded strings throughout this app:

  • Game states: "start", "play"
  • Keys that players have pressed to move the paddles or start the game: "w", "s","ArrowUp", "ArrowDown", "Enter"
  • And even some hard-coded numbers: 0 and 1 are used to represent the four directions that the ball can move

Strings are great to represent state concepts: they are clear and expressive indicators of what a state change signifies. I would argue that numbers are not as expressive as strings for communicating state changes. The usage of numbers for ball direction is kind of "boolean" in nature: 0 for "up", 1 for "down", but you have to study the code closely to understand what 0 actually means in the context in which it's being used. A string that clearly states "up" or "down" doesn't require close study to understand.

So! We've got some good strategies for representing state change, and some strategies that would benefit from refactoring to strings, but we can do better still. Seeing as the strings are hard-coded, it makes it easier to misspell them, or forget to update all of the instances of a hardcoded string if you decide to refactor the string in the future.

We can make our own home-rolled Typescript-style enums to save these strings in objects. This lets us divide the strings up into categories (states, movement keys, ball directions), and makes them easy and foolproof to update or expand if we need to:

const MovementKeys = {
P1_UP: "w",
P1_DOWN: "s",
P2_UP: "ArrowUp",
P2_DOWN: "ArrowDown",
};

const Keys = {
ENTER: "Enter",
...MovementKeys,
};

const GameStates = {
START: "start",
PLAY: "play",
};

const BallDirections = {
UP: "up",
DOWN: "down",
LEFT: "left",
RIGHT: "right",
};

// usage examples: 
let gameState = GameStates.START;

if (e.key == MovementKeys.P1_UP) {
// move p1 paddle up
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
let paddle_1 = document.querySelector('.paddle_1');
let paddle_2 = document.querySelector('.paddle_2');
let board = document.querySelector('.board');
let initial_ball = document.querySelector('.ball');
let ball = document.querySelector('.ball');
let score_1 = document.querySelector('.player_1_score');
let score_2 = document.querySelector('.player_2_score');
let message = document.querySelector('.message');
let paddle_1_coord = paddle_1.getBoundingClientRect();
let paddle_2_coord = paddle_2.getBoundingClientRect();
let initial_ball_coord = ball.getBoundingClientRect();
let ball_coord = initial_ball_coord;
let board_coord = board.getBoundingClientRect();
let paddle_common =
document.querySelector('.paddle').getBoundingClientRect();

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

I like that all of your setup code is organized at the top of the file. By putting you DOM element selectors and state in one place, it makes it much easier to navigate between what your app is acting on and the functions that perform the actions.

However, you've got game state, DOM selectors, and game constants all mixed together in one heap. You can make your code even easier to understand for outsiders by organizing them into their own separate categories, separated by newlines.

Additionally, many things which are effectively immutable (your DOM elements, the bounding rects of the board and paddle) are defined with let variables. let implies that a variable will change over the runtime of a program, and const signifies that it won't. If you define your immutable variables with const and your mutable variables with let, you can better communicate to the reader what is static and what is fluid.

// DOM elements
const paddle_1 = document.querySelector(".paddle_1");
const paddle_2 = document.querySelector(".paddle_2");
const board = document.querySelector(".board");
const initial_ball = document.querySelector(".ball");
const ball = document.querySelector(".ball");
const score_1 = document.querySelector(".player_1_score");
const score_2 = document.querySelector(".player_2_score");
const message = document.querySelector(".message");
const paddle_common = document.querySelector(".paddle").getBoundingClientRect();

// constants
const board_coord = board.getBoundingClientRect();
const initial_ball_coord = ball.getBoundingClientRect();

// state
let gameState = GameStates.START;
let ball_coord = initial_ball_coord;
let paddle_1_coord = paddle_1.getBoundingClientRect();
let paddle_2_coord = paddle_2.getBoundingClientRect();
``
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that all of your setup code is organized at the top of the file. By putting you DOM element selectors and state in one place, it makes it much easier to navigate between what your app is acting on and the functions that perform the actions.

However, you've got game state, DOM selectors, and game constants all mixed together in one heap. You can make your code even easier to understand for outsiders by organizing them into their own separate categories, separated by newlines.

Additionally, many things which are effectively immutable (your DOM elements, the bounding rects of the board and paddle) are defined with let variables. let implies that a variable will change over the runtime of a program, and const signifies that it won't. If you define your immutable variables with const and your mutable variables with let, you can better communicate to the reader what is static and what is fluid.

// DOM elements
const paddle_1 = document.querySelector(".paddle_1");
const paddle_2 = document.querySelector(".paddle_2");
const board = document.querySelector(".board");
const initial_ball = document.querySelector(".ball");
const ball = document.querySelector(".ball");
const score_1 = document.querySelector(".player_1_score");
const score_2 = document.querySelector(".player_2_score");
const message = document.querySelector(".message");
const paddle_common = document.querySelector(".paddle").getBoundingClientRect();

// constants
const board_coord = board.getBoundingClientRect();
const initial_ball_coord = ball.getBoundingClientRect();

// state
let gameState = GameStates.START;
let ball_coord = initial_ball_coord;
let paddle_1_coord = paddle_1.getBoundingClientRect();
let paddle_2_coord = paddle_2.getBoundingClientRect();
``
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

let dx = Math.floor(Math.random() * 4) + 3;

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

These random numbers you're generating are used repeatedly in numerous places: lines 18-21, 30-33, 88-89, and 97-98 all use the same random number generator functions. If you wrap this repetitive code up in a function, you can:

  • give the function a name (like randomCoordinate or randomDirection), which will help to express what those random numbers represent, and will aid comprehension for others who are reading your code.
  • give yourself an easy way to update all instances of those random numbers. Maybe in the future you decide that you don't want to round down, or maybe you want to increase or decrease what you're multiplying or adding to your random coordinates to add more variation to ball placement between rounds. As it is, you'd have to update all of your random number functions in many places, and you could miss some instances if you're not diligent. If those parameters are wrapped up in a function, however, then you only have to update them in one place and you're done!
const randomCoord = () => Math.floor(Math.random() * 4) + 3;
const randomDirection = () => Math.floor(Math.random() * 2);

let dx = randomCoord();
let dy = randomCoord();
let dxd = randomDirection();
let dyd = randomDirection();

// etc
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These random numbers you're generating are used repeatedly in numerous places: lines 18-21, 30-33, 88-89, and 97-98 all use the same random number generator functions. If you wrap this repetitive code up in a function, you can:

  • give the function a name (like randomCoordinate or randomDirection), which will help to express what those random numbers represent, and will aid comprehension for others who are reading your code.
  • give yourself an easy way to update all instances of those random numbers. Maybe in the future you decide that you don't want to round down, or maybe you want to increase or decrease what you're multiplying or adding to your random coordinates to add more variation to ball placement between rounds. As it is, you'd have to update all of your random number functions in many places, and you could miss some instances if you're not diligent. If those parameters are wrapped up in a function, however, then you only have to update them in one place and you're done!
const randomCoord = () => Math.floor(Math.random() * 4) + 3;
const randomDirection = () => Math.floor(Math.random() * 2);

let dx = randomCoord();
let dy = randomCoord();
let dxd = randomDirection();
let dyd = randomDirection();

// etc
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
let dy = Math.floor(Math.random() * 4) + 3;
let dxd = Math.floor(Math.random() * 2);
let dyd = Math.floor(Math.random() * 2);

document.addEventListener('keydown', (e) => {

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

The nature of the keydown event listener we're working with here has divided the code within into two nice parts: code that starts the game, and code that moves the paddle. Including both pieces as part of one big callback, however, makes things just a little bit harder to navigate.

If we take these individual if blocks and break them up into functions, we can simultaneously give names to what's being done and make it easier for future readers to ignore what they're not interested in. Need to know about the mechanics for how the rounds are started? Look at startRound. Need to change how the paddles move? Look in movePaddle.

const handleKeyPress = (e) => {
if (e.key === Keys.ENTER) {
startRound();
}

if (gameState === GameStates.PLAY && Object.values(MovementKeys).includes(e.key)) {
movePaddle(e)
}
};
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nature of the keydown event listener we're working with here has divided the code within into two nice parts: code that starts the game, and code that moves the paddle. Including both pieces as part of one big callback, however, makes things just a little bit harder to navigate.

If we take these individual if blocks and break them up into functions, we can simultaneously give names to what's being done and make it easier for future readers to ignore what they're not interested in. Need to know about the mechanics for how the rounds are started? Look at startRound. Need to change how the paddles move? Look in movePaddle.

const handleKeyPress = (e) => {
if (e.key === Keys.ENTER) {
startRound();
}

if (gameState === GameStates.PLAY && Object.values(MovementKeys).includes(e.key)) {
movePaddle(e)
}
};
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
if (e.key == 'Enter') {
gameState = gameState == 'start' ? 'play' : 'start';
if (gameState == 'play') {
message.innerHTML = 'Game Started';
message.style.left = 42 + 'vw';
requestAnimationFrame(() => {
dx = Math.floor(Math.random() * 4) + 3;
dy = Math.floor(Math.random() * 4) + 3;
dxd = Math.floor(Math.random() * 2);
dyd = Math.floor(Math.random() * 2);
moveBall(dx, dy, dxd, dyd);
});
}
}
if (gameState == 'play') {
if (e.key == 'w') {

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

There's a lot of repeated code in here for moving paddles up and down, with only slight variations between the various implementations (which paddle we're moving, which direction, etc.). To make this code a little shorter, and to make it easier to understand at a glance, I'd suggest abstracting all this into movePaddleDown and movePaddleUp functions. That, combined with a little bit of thoughtful variable naming and some nice ES6 string-template code, can make this a lot easier to grok and work with:

// an example implementation for movePaddleUp
// movePaddleDown would be the same thing, but with all the math appropriately flipped around
const movePaddleUp = (paddle) => {
const nextPosition = Math.max(
board_coord.top,
paddle.top - window.innerHeight * movementInterval
);

return `${nextPosition}px`;
};

// Example function calls: 
if (e.key == MovementKeys.P1_UP) {
paddle_1.style.top = movePaddleUp(paddle_1_coord);
}

if (e.key == MovementKeys.P1_DOWN) {
paddle_1.style.top = movePaddleDown(paddle_1_coord);
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of repeated code in here for moving paddles up and down, with only slight variations between the various implementations (which paddle we're moving, which direction, etc.). To make this code a little shorter, and to make it easier to understand at a glance, I'd suggest abstracting all this into movePaddleDown and movePaddleUp functions. That, combined with a little bit of thoughtful variable naming and some nice ES6 string-template code, can make this a lot easier to grok and work with:

// an example implementation for movePaddleUp
// movePaddleDown would be the same thing, but with all the math appropriately flipped around
const movePaddleUp = (paddle) => {
const nextPosition = Math.max(
board_coord.top,
paddle.top - window.innerHeight * movementInterval
);

return `${nextPosition}px`;
};

// Example function calls: 
if (e.key == MovementKeys.P1_UP) {
paddle_1.style.top = movePaddleUp(paddle_1_coord);
}

if (e.key == MovementKeys.P1_DOWN) {
paddle_1.style.top = movePaddleDown(paddle_1_coord);
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
paddle_1.style.top =
Math.max(
board_coord.top,
paddle_1_coord.top - window.innerHeight * 0.06
) + 'px';
paddle_1_coord = paddle_1.getBoundingClientRect();
}
if (e.key == 's') {
paddle_1.style.top =
Math.min(
board_coord.bottom - paddle_common.height,
paddle_1_coord.top + window.innerHeight * 0.06
) + 'px';
paddle_1_coord = paddle_1.getBoundingClientRect();
}

if (e.key == 'ArrowUp') {
paddle_2.style.top =
Math.max(
board_coord.top,
paddle_2_coord.top - window.innerHeight * 0.1

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

This is a prime example of how hard-coded values used repetitively across a codebase can quietly introduce what I will call "unintended functionality". Nothing is "broken" per se, so you couldn't properly call it a "bug" in the sense that most people would understand that term, but I don't think you intended to give player two such a huge structural advantage over player one.

Player two's paddle moves almost twice as fast as player one's paddle (0.1px per keystroke for player two, 0.06px for player one). If this movement interval had originally been coded as a constant, with the value living and being updated from a single point, then this wouldn't have made it into the final version.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a prime example of how hard-coded values used repetitively across a codebase can quietly introduce what I will call "unintended functionality". Nothing is "broken" per se, so you couldn't properly call it a "bug" in the sense that most people would understand that term, but I don't think you intended to give player two such a huge structural advantage over player one.

Player two's paddle moves almost twice as fast as player one's paddle (0.1px per keystroke for player two, 0.06px for player one). If this movement interval had originally been coded as a constant, with the value living and being updated from a single point, then this wouldn't have made it into the final version.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
) + 'px';
paddle_2_coord = paddle_2.getBoundingClientRect();
}
if (e.key == 'ArrowDown') {
paddle_2.style.top =
Math.min(
board_coord.bottom - paddle_common.height,
paddle_2_coord.top + window.innerHeight * 0.1
) + 'px';
paddle_2_coord = paddle_2.getBoundingClientRect();
}
}
});

function moveBall(dx, dy, dxd, dyd) {
if (ball_coord.top <= board_coord.top) {
dyd = 1;
}
if (ball_coord.bottom >= board_coord.bottom) {
dyd = 0;
}
if (
ball_coord.left <= paddle_1_coord.right &&
ball_coord.top >= paddle_1_coord.top &&
ball_coord.bottom <= paddle_1_coord.bottom
) {
dxd = 1;
dx = Math.floor(Math.random() * 4) + 3;
dy = Math.floor(Math.random() * 4) + 3;
}
if (
ball_coord.right >= paddle_2_coord.left &&
ball_coord.top >= paddle_2_coord.top &&
ball_coord.bottom <= paddle_2_coord.bottom
) {
dxd = 0;
dx = Math.floor(Math.random() * 4) + 3;
dy = Math.floor(Math.random() * 4) + 3;
}
if (
ball_coord.left <= board_coord.left ||
ball_coord.right >= board_coord.right
) {
if (ball_coord.left <= board_coord.left) {
score_2.innerHTML = +score_2.innerHTML + 1;
} else {
score_1.innerHTML = +score_1.innerHTML + 1;
}
gameState = 'start';

ball_coord = initial_ball_coord;
ball.style = initial_ball.style;
message.innerHTML = 'Press Enter to Play Pong';
message.style.left = 38 + 'vw';
return;
}
ball.style.top = ball_coord.top + dy * (dyd == 0 ? -1 : 1) + 'px';

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

There's a lot of stuff going on in here:

  • Math to determine the next top and left position of the ball
  • Logic to determine whether the ball should move up, down, left or right on the next frame
  • Coercion of a number value to a string by way of adding a number to a string literal
  • Setting the measurement unit for the value derived by all of the above (px)
  • And finally, setting the actual property of the ball's DOM object to the final string so we can move the ball

It is terse, which is one dimension by which you can measure the readability of code, but I'd also call it opaque in meaning and in function, which makes it less readable.

A lot of assumed knowledge of what these variables mean and experience with the weird, hairy corners of JS – specifically how the language handles things like combining numbers with strings using the + operator – is required to grasp the functionality in full.

Using a couple variable names, some of the constants we mentioned in another comment, and ES6 string templating, we can make this a lot more understandable while only making the code a little bit longer:

const nextTopCoord = ball_coord.top + dy * (dyd === BallDirections.UP ? -1 : 1);
ball.style.top = `${nextTopCoord}px`;

// same thing for the next left coordinate
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of stuff going on in here:

  • Math to determine the next top and left position of the ball
  • Logic to determine whether the ball should move up, down, left or right on the next frame
  • Coercion of a number value to a string by way of adding a number to a string literal
  • Setting the measurement unit for the value derived by all of the above (px)
  • And finally, setting the actual property of the ball's DOM object to the final string so we can move the ball

It is terse, which is one dimension by which you can measure the readability of code, but I'd also call it opaque in meaning and in function, which makes it less readable.

A lot of assumed knowledge of what these variables mean and experience with the weird, hairy corners of JS – specifically how the language handles things like combining numbers with strings using the + operator – is required to grasp the functionality in full.

Using a couple variable names, some of the constants we mentioned in another comment, and ES6 string templating, we can make this a lot more understandable while only making the code a little bit longer:

const nextTopCoord = ball_coord.top + dy * (dyd === BallDirections.UP ? -1 : 1);
ball.style.top = `${nextTopCoord}px`;

// same thing for the next left coordinate
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .