2
\$\begingroup\$

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

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.