Been learning some react and typescript and wanted to get some feedback on this small project I did. Feel free to be as nit-picky as you like.
import './App.css'
import Button from './components/button';
import Grid from './components/grid'
import { useEffect, useRef, useState } from 'react';
// This is me trying to make conway's game of line in typescript + React. don't ask me why
function App() {
// set size of grid
const cols = 9;
const rows = 10;
// Create the 2d array to manage cell state
const createGrid = (rows: number, cols: number) => {
const cell_states: boolean[][] = [];
for (let i = 0; i < rows; i++) {
cell_states[i] = [];
for (let j = 0; j < cols; j++) {
cell_states[i][j] = false
}
}
return cell_states;
}
const [cell_states, setCellStates] = useState<boolean[][]>(createGrid(rows, cols));
// indexes of current cell
const [rowIdx, setRowIdx] = useState(0);
const [colIdx, setColIdx] = useState(0);
// loop is running
const [isRunning, setIsRunning] = useState(false);
// flipped ref
const flipped = useRef(false);
const sign = useRef<number>(1);
// 1 if forwards; -1 if backwards
const backwards = useRef<number>(1);
const end_tup: [row: number, col: number] = cols % 2 === 0 ? [0, cols - 1]: [rows - 1, cols - 1];
const end = useRef<[rows: number, cols: number]>(end_tup);
// useEffect triggers during startup and when states in the dependency array changes
useEffect(() => {
// if it's not running i.e. button hasn't been pressed yet.
if(!isRunning) return;
// don't quite understand myself but we use timeouts to set a delay between to emulate animation
const timeoutId: number = setTimeout(() => {
// tick when flipped is on will just remove the row changes from the previous tick so that in the next tick it starts at end;
if (flipped.current) {
flipped.current = false;
setRowIdx(prev => prev += (1 * sign.current));
return;
}
// toggle cell function; dead -> alive, alive -> dead
const toggle = (row: number, col: number) => {
setCellStates((prev) => {
const newGrid = [...prev];
newGrid[row] = [...prev[row]];
newGrid[row][col] = !newGrid[row][col];
return newGrid;
})
}
toggle(rowIdx, colIdx);
// row and column update
if ((rowIdx < rows - 1 && sign.current === 1)|| rowIdx > 0 && sign.current === -1)
{
setRowIdx(prev => prev += (1 * sign.current)); // every non row edge cell
}
else if (end.current[0] === rowIdx && end.current[1] === colIdx) // when row,col reaches the end square
{
// switch the signs, and flow to backwards
backwards.current = backwards.current * -1;
sign.current = sign.current * -1;
// set end as the opposite diagonal coordinate
const end_row: number = cols % 2 === 0 ? 0: (end.current[0] * -1) + rows - 1;
const end_col: number = (end.current[1] * -1) + cols - 1;
end.current = [end_row, end_col];
// set flipped to true for the end edge case
flipped.current = true;
// this will set the row index out of bounds in the next tick but needed to retrigger rerender
setRowIdx(prev => prev += (-1 * sign.current))
} else
{
// moving to the next column
setColIdx(prev => prev += (1 * backwards.current));
sign.current = sign.current * -1;
}
}, 50);
return () => clearTimeout(timeoutId);
}, [colIdx, isRunning, rowIdx]);
const handleClick = () => {
setCellStates(createGrid(rows, cols));
setRowIdx(0);
setColIdx(0);
flipped.current = false;
sign.current = 1;
backwards.current = 1;
setIsRunning(true);
};
return (
<>
<Grid col_size={cols} rows={rows} states={cell_states} ></Grid>
<Button OnClick={handleClick}></Button>
</>
)
}
export default App
1 Answer 1
TS is not my favorite language, so this answer will likely only scratch the surface.
Style
If/else braces are placed inconsistently. Either do
if (x) {
// ...
} else {
// ...
}
or
if (x)
{
// ...
}
else
{
// ...
}
or something else, but do it consistently. Braces placement should not catch reader's attention.
Similar remarks apply to vertical whitespace (why does setTimeout function end with two blank lines?) and semicolons. Consider employing a formatter like prettier or biome to never think about this again. And a linter (eslint/oxc/biome) on top of that - linter can detect some non-obvious bugs in your code and make it more consistent.
Trailing comments
Please don't use trailing comments:
else if (end.current[0] === rowIdx && end.current[1] === colIdx) // when row,col reaches the end square
this is too far to the right, it'd be easier to read this comment on its own line.
Unnecessary comments
Comments should explain the why, not how. Here's why all of the following are redundant and should be removed:
// if it's not running i.e. button hasn't been pressed yet.
if(!isRunning) return;
// flipped ref
const flipped = useRef(false);
// set size of grid
const cols = 9;
const rows = 10;
Naming
It's close to style nits, but probably deserves its own section. Use consistent casing - the community standard for TS is camelCase variables and functions. This hurts:
const [cell_states, setCellStates] = ...
However, consistency is not the only aspect. Try to pick short and descriptive names that explain the use of the variable as close as possible. backwards could be directionSign, because it's a number, not a flag (for a flag the better name would be isBackwards).
Layout
The component size is reasonable, but consider moving at least every function not depending on state to the top level. This makes testing and reading the component easier. createGrid is a perfect candidate, main block of toggle is also good.
Simple
This:
const createGrid = (rows: number, cols: number) => {
const cell_states: boolean[][] = [];
for (let i = 0; i < rows; i++) {
cell_states[i] = [];
for (let j = 0; j < cols; j++) {
cell_states[i][j] = false
}
}
return cell_states;
}
is OK. But it could be
const createGrid = (rows: number, cols: number): boolean[][] => {
return new Array(rows).fill(0).map(
() => new Array(cols).fill(false)
);
}
Type annotations
This is redundant, usually we only annotate local variables to fix imprecise inference:
const end_row: number = cols % 2 === 0 ? 0: (end.current[0] * -1) + rows - 1;
But this is somewhat lacking, explicit return type on non-inline functions helps keep their boundaries:
const createGrid = (rows: number, cols: number) => { /* ... */ }
Avoid non-trivial state initializers
With the following a new grid is created every render only to be immediately destroyed:
const [cell_states, setCellStates] = useState<boolean[][]>(createGrid(rows, cols));
This will only call the factory when needed:
const [cell_states, setCellStates] = useState<boolean[][]>(() => createGrid(rows, cols));
Semantic types
boolean[][] is not the most descriptive type I've ever seen. Consider doing
type Board = boolean[][]
and reusing it instead. Similar with
type Point = [row: number, col: number];
// ...
const end_tup: Point = cols % 2 === 0 ? [0, cols - 1]: [rows - 1, cols - 1];
const end = useRef<Point>(end_tup);
Arithmetics
Perhaps I do understand your desire to write this:
setRowIdx(prev => prev += (1 * sign.current));
but to me this mul-by-one looks too weird. I'd prefer to remove it altogether and also replace
- setRowIdx(prev => prev += (-1 * sign.current))
+ setRowIdx(prev => prev - sign.current)
and
- sign.current = sign.current * -1;
+ sign.current = -sign.current;
Also note that there's no need to use augmented ops here, prev is destroyed anyway, mutation is only confusing.