Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alphonso's code review #24

Open
camelPhonso opened this issue Oct 18, 2023 · 0 comments
Open

Alphonso's code review #24

camelPhonso opened this issue Oct 18, 2023 · 0 comments

Comments

@camelPhonso
Copy link

  • The logic going on in this game is intense and I love how far it's come
  • Ternaries are beautiful, but they're not the right tool for everything:
<div
	key={colIndex}
	className={`cell ${
		// this evaluation is overly complicated and feels uncertain
			cell === 'B' || cell === 'A' ? 'ship-cell' : ''
		}`
	}
>

Short-circuit Evaluations are also friends!!

<div
	key={colIndex}
	className={`cell ${
		// this evaluation is simple and clear
		// you can evaluate `cell` as a boolean because it initializes as `null`
			cell && 'ship-cell'
		}`
	}
>
  • Watch out for your file structure / organisation:
    ![[Screenshot 2023-10-18 at 15.37.17.png]]
    Two Game.jsx files, tsk tsk...
  • Your logic on placeShip.jsx is honestly impressive. I was also partial to your clean up by packaging some logic into helper functions for readability. One other suggestion I would make is maybe bundling props into objects just for the sake of removing noise from the screen:
const newShip = {
	row: Math.floor(Math.random() * numRows), // did you notice these two 
	col: Math.floor(Math.random() * numCols), // could just be numCells ??
	orientation: Math.floor(Math.random() * 2)
}

if (canPlaceShip(newShip, actionProps)) {
// here I've also packaged the rest of the props that were drilled in from Game.jsx
	for (let i = 0; i < shipLength; i++) {
		newShip.orientation === 0 ?  // this was a ternary waiting to happen
		board[newShip.row][newShip.col + i] = 'B' :
		board[newShip.row + i][newShip.col] = 'A' ;
}
  • On another note, that file did not need to be a .jsx since it doesn't return a component.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant