Implement Tetris game with board and tetromino logic #55
Conversation
Reviewer's Guide by SourceryThis pull request implements a Tetris game within the application. It introduces new components for the game board, tetrominoes, and game logic. It also adds a new route and page for the Tetris game, making it accessible through the main application. Sequence diagram for Tetris game movement and collisionsequenceDiagram
participant User
participant Tetris
participant TetrominoUtils
participant GameBoard
User->>Tetris: Press arrow key
alt Left/Right Movement
Tetris->>TetrominoUtils: checkCollision()
TetrominoUtils-->>Tetris: collision result
alt No Collision
Tetris->>Tetris: Update position
end
else Down Movement
Tetris->>TetrominoUtils: checkCollision()
TetrominoUtils-->>Tetris: collision result
alt No Collision
Tetris->>Tetris: Update position
else Collision
Tetris->>GameBoard: Lock tetromino
Tetris->>TetrominoUtils: getRandomTetromino()
TetrominoUtils-->>Tetris: new tetromino
Tetris->>Tetris: Reset position
end
end
Class diagram for Tetris game componentsclassDiagram
class Tetris {
-board: number[][]
-tetromino: Tetromino
-position: Position
-gameOver: boolean
+moveTetromino(direction: number)
+dropTetromino()
+rotateTetromino()
}
class GameBoard {
+board: number[][]
+render()
}
class TetrominoComponent {
+shape: number[][]
+position: Position
+render()
}
class Position {
+x: number
+y: number
}
class TetrominoUtils {
+checkCollision(tetromino: number[][], board: number[][], position: Position): boolean
+createBoard(): number[][]
+getRandomTetromino(): Tetromino
}
Tetris --> GameBoard
Tetris --> TetrominoComponent
Tetris ..> TetrominoUtils
TetrominoComponent --> Position
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Summary
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JuhQ - I've reviewed your changes - here's some feedback:
Overall Comments:
- There's a TODO comment stating 'does not work' - please fix any outstanding issues and remove this comment before merging
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const [position, setPosition] = useState({ x: 3, y: 0 }); | ||
| const [gameOver, setGameOver] = useState(false); | ||
|
|
||
| const moveTetromino = useCallback( |
There was a problem hiding this comment.
issue: Add bounds checking to prevent pieces from moving off the board edges
The current collision detection only checks for collisions with other pieces and the bottom. You should also check if position.x + tetromino width is within the board bounds.
| } | ||
| }, [position, tetromino.shape, board]); | ||
|
|
||
| const rotateTetromino = useCallback(() => { |
There was a problem hiding this comment.
suggestion: Implement wall kick behavior when rotating pieces near walls
When a rotation would cause a collision, try shifting the piece left or right to make it fit - this is standard Tetris behavior known as wall kicks.
| if (!checkCollision(tetromino.shape, board, newPosition)) { | ||
| setPosition(newPosition); | ||
| } else { | ||
| // Lock the tetromino and generate a new one |
There was a problem hiding this comment.
issue: Implement row clearing mechanics when a row is completely filled
After locking a piece, check for and remove any completed rows, then shift all rows above downward.
| color: string; | ||
| } | ||
|
|
||
| // TODO: does not work |
There was a problem hiding this comment.
issue (complexity): Consider extracting the game logic into a custom hook to separate it from the rendering logic.
The component's complexity can be reduced by:
- Extracting game logic into a custom hook
- Removing unnecessary useCallback wrappers
- Simplifying state management
Example implementation:
// useTetrisGame.ts
export function useTetrisGame() {
const [board, setBoard] = useState(createBoard());
const [tetromino, setTetromino] = useState<Tetromino>(getRandomTetromino());
const [position, setPosition] = useState({ x: 3, y: 0 });
const [gameOver, setGameOver] = useState(false);
function moveTetromino(direction: number) {
const newPosition = { ...position, x: position.x + direction };
if (!checkCollision(tetromino.shape, board, newPosition)) {
setPosition(newPosition);
}
}
function dropTetromino() {
const newPosition = { ...position, y: position.y + 1 };
if (!checkCollision(tetromino.shape, board, newPosition)) {
setPosition(newPosition);
return;
}
const newBoard = [...board];
tetromino.shape.forEach((row, y) => {
row.forEach((cell, x) => {
if (cell !== 0) {
newBoard[position.y + y][position.x + x] = cell;
}
});
});
setBoard(newBoard);
setTetromino(getRandomTetromino());
setPosition({ x: 3, y: 0 });
if (checkCollision(tetromino.shape, newBoard, { x: 3, y: 0 })) {
setGameOver(true);
}
}
return {
board,
tetromino,
position,
gameOver,
moveTetromino,
dropTetromino,
// ... other methods
};
}
// Tetris.tsx
const Tetris: React.FC = () => {
const game = useTetrisGame();
useEffect(() => {
const handleKeyDown = (event: KeyboardEvent) => {
switch (event.key) {
case "ArrowLeft": game.moveTetromino(-1); break;
case "ArrowRight": game.moveTetromino(1); break;
case "ArrowDown": game.dropTetromino(); break;
case "ArrowUp": game.rotateTetromino(); break;
}
};
window.addEventListener("keydown", handleKeyDown);
return () => window.removeEventListener("keydown", handleKeyDown);
}, [game]);
return (
<div className="game">
{game.gameOver ? <div className="game-over">Game Over</div> : null}
<GameBoard board={game.board} />
<Tetromino shape={game.tetromino.shape} position={game.position} />
</div>
);
};This approach:
- Separates game logic from rendering
- Removes unnecessary callback wrapping
- Makes the component's responsibility clear
- Improves testability of game logic
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| grid-template-rows: repeat(20, 20px); | ||
| grid-template-columns: repeat(10, 20px); |
There was a problem hiding this comment.
The grid dimensions are defined with fixed pixel values (20px), which limits the responsiveness and scalability of the game board. Consider using relative units like vw, vh, or percentages to make the grid responsive and adaptable to different screen sizes.
For example, you could modify the grid definition to use viewport units:
.game-board {
grid-template-rows: repeat(20, 1vw);
grid-template-columns: repeat(10, 1vw);
}| {board.map((row, rowIndex) => ( | ||
| <div key={rowIndex} className="row"> | ||
| {row.map((cell, cellIndex) => ( | ||
| <div | ||
| key={cellIndex} | ||
| className={`cell ${cell ? "filled" : ""}`} | ||
| ></div> | ||
| ))} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
The use of nested .map() functions for rendering the game board can lead to performance issues, especially for larger boards or frequent updates. Each .map() call creates a new function instance, which can be costly in terms of performance.
Recommended Solution: Consider using a memoization technique or React's useMemo hook to cache the board's rendered output unless the board prop changes. This can prevent unnecessary re-renders and improve performance.
| key={cellIndex} | ||
| className={`cell ${cell ? "filled" : ""}`} |
There was a problem hiding this comment.
Using cellIndex as a key for cells might lead to issues with unnecessary re-renders or DOM manipulations if the board structure changes (e.g., rows are added or removed), as the indices might not uniquely identify a cell over re-renders.
Recommended Solution: Use a more stable identifier for the key, such as a combination of rowIndex and cellIndex (e.g., `key={
owIndex-cellIndex}"). This ensures that each cell maintains a unique identifier across re-renders, improving React's ability to manage DOM updates efficiently.
| const newBoard = [...board]; | ||
| tetromino.shape.forEach((row, y) => { | ||
| row.forEach((cell, x) => { | ||
| if (cell !== 0) { | ||
| newBoard[position.y + y][position.x + x] = cell; | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The operation const newBoard = [...board]; on line 35 only creates a shallow copy of the board array. Since board is an array of arrays, this means the inner arrays are not copied, and modifying newBoard will mutate the original board state. This can lead to bugs and performance issues.
Recommended Solution:
Use a deep copy method to ensure that the inner arrays are also copied. You can achieve this by using map to copy each inner array:
const newBoard = board.map(innerArray => [...innerArray]);| if (checkCollision(tetromino.shape, newBoard, { x: 3, y: 0 })) { | ||
| setGameOver(true); | ||
| } |
There was a problem hiding this comment.
The game over check is performed after setting a new tetromino, which might not be the most efficient or safest point to perform this check. This could potentially lead to a scenario where the game over state is set but the board state is not correctly updated to reflect this, leading to inconsistencies in the UI and game logic.
Recommended Solution:
Consider checking for game over conditions before setting the new tetromino. This way, you can ensure that all game state updates are consistent and reflect the actual state of the game:
if (checkCollision(tetromino.shape, board, { x: 3, y: 0 })) {
setGameOver(true);
} else {
setTetromino(getRandomTetromino());
setPosition({ x: 3, y: 0 });
}| style={{ | ||
| position: "absolute", | ||
| top: position.y * 20, | ||
| left: position.x * 20, | ||
| }} |
There was a problem hiding this comment.
Using inline styles in React components can lead to performance issues because the style object is recreated on every render. This can cause unnecessary re-renders of the DOM elements. Consider using useMemo for the style object or moving the styles to a CSS class to prevent this issue.
Recommended Solution:
const style = useMemo(() => ({
position: 'absolute',
top: position.y * 20,
left: position.x * 20
}), [position.x, position.y]);Then use this style in your component.
| top: position.y * 20, | ||
| left: position.x * 20, |
There was a problem hiding this comment.
The positioning of the Tetromino uses hardcoded multipliers (20) for the top and left styles. This approach reduces the flexibility of the component, especially if the size of the cells needs to be adjusted or made responsive. Consider defining these values as props or using a scaling factor that can be adjusted based on the component's environment.
Recommended Solution:
Add a new prop, e.g., scale, and use it to calculate the position:
const scale = props.scale || 20; // default to 20 if not provided
const style = useMemo(() => ({
position: 'absolute',
top: position.y * scale,
left: position.x * scale
}), [position.x, position.y, scale]);
src/components/Tetris/utils.ts
Outdated
| !board[position.y + y] || | ||
| !board[position.y + y][position.x + x] || | ||
| board[position.y + y][position.x + x] !== 0 |
There was a problem hiding this comment.
The checkCollision function potentially accesses out-of-bound indices on the board array without explicit handling. This can lead to runtime errors if position.y + y or position.x + x exceeds the board dimensions. To prevent this, consider adding bounds checking before accessing these indices.
Suggested Solution:
if ((position.y + y >= board.length) || (position.x + x >= board[position.y + y].length)) {
return true; // Assuming out-of-bounds is treated as a collision.
}| tetrominos[Math.floor(Math.random() * tetrominos.length)]; | ||
| return TETROMINOS[randTetromino]; |
There was a problem hiding this comment.
The getRandomTetromino function uses Math.random() for generating a random index, which is suitable for non-security-critical applications like games. However, it's important to be aware that Math.random() does not provide cryptographic security and can be predictable. This is generally not an issue for game mechanics but could be relevant if the randomness affects competitive gameplay or if used in a security-sensitive context.
Note: No change is necessary for the current context unless the application requirements specify higher security for random number generation.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
Add a complete Tetris game implementation, including game board rendering, tetromino movement, and collision detection. Integrate the game into the application with a dedicated route and page.
Summary by Sourcery
Implement the Tetris game.
New Features:
Tests: