2
\$\begingroup\$

I have been learning webdevelopment at Freecodcamp and have therefore been writing a lot of javascript by myself. They have a code-review chatroom, but the people in that chatroom seem to review the final product and not the code itself.

So, I am searching for some constructive criticism about the structure and style of my code.

The code shown below is a class from a Game of Life game as part of the Freecodecamp course and does work. It is written in Javascript and uses React. There is more code for this project, but I don't think it is necessary to show all of the code, for you to critique my code structure and style. If it the rest of the code is required I can of course post it as well.

class GameOfLife extends Component {
  constructor(props) {
    super(props)
    this.toggleGameState = this.toggleGameState.bind(this);
    this.stepGame = this.stepGame.bind(this);
    this.squareClicked = this.squareClicked.bind(this);
    this.clearGrid = this.clearGrid.bind(this);
    this.generateRandomGrid = this.generateRandomGrid.bind(this);
    this.toggleSpeed = this.toggleSpeed.bind(this);
    this.state = {
      cellRows: generateRandomCellDistribution({height: 50, width: 50}), 
      size: {height:50, width:50}, 
      gameState: 'playing', speed: 1,
      generation: 0
    };
  }

  componentDidMount() {
    this.setState({gamteState: 'playing', interval: setInterval(this.stepGame, 200/this.state.speed)});
  }

  toggleGameState() {
    var newState = this.state.gameState === 'playing' ? 'paused' : 'playing';
    if(newState === 'playing') {
      this.setState({gameState: newState, interval: setInterval(this.stepGame, 200/this.state.speed)});
    } else {
      this.setState({'gameState': newState});
      clearInterval(this.state.interval);
    }
  }

  squareClicked(row, column) {
    var cellRows = this.state.cellRows.slice();
    var newState = cellRows[row][column].state === 'alive' ? 'dead' : 'alive';
    cellRows[row][column] = {state: newState};
    this.setState({cellRows: cellRows});
  }

  generateRandomGrid() {
    this.setState({cellRows: generateRandomCellDistribution(this.state.size), generation: 0})
  }

  clearGrid() {
    this.setState({cellRows: getBlankCellRows(this.state.size), generation: 0});
  }

  toggleSpeed(){
    var newSpeed = this.state.speed < 4 ? this.state.speed+1 : 1;
    clearInterval(this.state.interval);
    this.setState({speed: newSpeed, interval: setInterval(this.stepGame, 200/newSpeed)});
  }

  stepGame() {
    var newCellRows = getBlankCellRows(this.state.size);
    var currentCellRows = this.state.cellRows.slice();
    var aliveNeighbours = getAliveNeighbours(currentCellRows);

    for(var i = 0; i < currentCellRows.length; i++) {
      for(var q = 0; q < currentCellRows[i].length; q++) {
        newCellRows[i][q].state = getNewState(currentCellRows[i][q].state, getAliveNeighbours(currentCellRows, i, q));
      }
    }

    this.setState({cellRows: newCellRows, generation: this.state.generation + 1});
  }

  render(){
    return(
      <div>
        <div className="ui-container"> 
          <button className="button-theme" onClick={this.toggleGameState}>{this.state.gameState === 'playing' ? 'Pause' : 'Start'}</button>
          <button className="button-theme" onClick={this.clearGrid}>Clear grid</button>
          <button className="button-theme" onClick={this.generateRandomGrid}>Random</button>
          <button className="button-theme" onClick={this.toggleSpeed}>Speed: {this.state.speed}x</button>
          <div className="generation-indicator">Generation: {this.state.generation}</div>
        </div>
        <Grid 
          gameState={this.state.gameState} 
          size={this.state.size} 
          cellRows={this.state.cellRows} 
          squareClicked={this.squareClicked}
        />
      </div>
    );
  }
}
\$\endgroup\$
1
\$\begingroup\$
// Good: Object properties are in a logical order 
// (alphabetical order isn't always best)
this.state = {
  cellRows: generateRandomCellDistribution({height: 50, width: 50}), 
  size: {height:50, width:50}, 
  gameState: 'playing', 
  speed: 1,       // This property should be on its own line like the rest.
  generation: 0
};

// Style should be consistent (Source: Code Complete)
// Object properties on their own lines like above
// (even as parameters).
componentDidMount() {
  this.setState({
    gamteState: 'playing',
    // No magic numbers (Source: Code Complete)
    // Why 200/speed? What does that represent?
    // Assign a variable above like var tickSpeed = 200/this.state.speed
    // Something to explain why you're doing the division.
    // Even better would be to put that in another function
    // to be used in all places you do this conversion. 
    interval: setInterval(this.stepGame, 200/this.state.speed)
  });
}

// Function name should describe what the function does
// (rather than what triggers it) (Source: Code Complete)
squareClicked(row, column)  -->  handleClickInSquare(row, column) or updateStateForCell(row, column)

// Same here. Function doesn't just return a random grid, 
// it changes the grid to be a random grid (affects state).
generateRandomGrid()  -->  setGridToRandomGrid() or randomizeGrid()

// This one is pretty good, but toggle usually refers to 
// something with two options like Caps Lock (Source: Google define).
// This is tricky and you might think up an even better name that
// includes the facts that you're increasing the speed, there are 
// several options, and it wraps around back to the first option.
toggleSpeed()  -->  cycleSpeed(), increaseSpeedOrWrap(), or selectNextFasterSpeed()

// It's better to select clear iterators although it's not
// uncommon to see i, j, and k. Not sure what q is, though.
// How about i --> row, q --> cell. (Source: Code Complete)
// I've changed the code to demonstrate my suggestions.
// I also recommend using currentCellGrid or similar
// to make it clear that you have a 2D array.
for(var row = 0; row < currentCellGrid.length; row++) {
  // Here you could add a variable to clarify what currentCellGrid[row] is.
  // Use more variables to document what the code is doing.
  // (Source: Code Complete)
  var currentRowCells = currentCellGrid[row];
  for(var cell = 0; cell < currentRowCells.length; cell++) {
    newCellGrid[row][cell].state = getNewState(currentRowCells[cell].state, getAliveNeighbours(currentCellGrid, row, cell));
  }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.