1

I'm building a tower of Hanoi game to get used to react. I have a state property called "disks", which is an array consisting of 3 arrays of length N (N being the total number of disks). I also have defined a state property "history" which is supposed to contain the history of the disks array like this:

  1. intially: history = [disks(Initial config)]
  2. After 1 move: history = [disks(Initial config), disks(after 1 move)]
  3. After 2 moves: history = [disks(Initial config), disks(after 1 move), disks(after 2 move)] etc.

However, after M moves, the history array looks like this:

history = [disks(after M moves), disks(after M moves), ... , disks(after M moves)].

I can't find my mistake. Would appreciate it if anyone had an idea what's going wrong. Here is the relevant code: 

constructor(props){
    super(props);
    let disks = [
      [],
      [],
      []
    ];
    //Initially all disks in first tower
    for(let i=0; i<props.numberOfDisks; i++){
      disks[0].push(i);
    }

    this.state = {
      disks : disks,
      selected : null,
      move: 0,
      history: [disks]
    };
  }

  handleClick(i){
    const disks = this.state.disks.slice();
    const history = this.state.history.slice();
    let move = this.state.move;
    let selected = this.state.selected;
    //if user has not previously selected a tower or selects the same tower again
    if(selected===null || i===selected){
      selected = disks[i].length>0 && i!==selected ? i : null;
      this.setState({
        selected : selected
      });
      return;
    }
    //Check if move is legal
    //index is at bottom is 0 and the largest disk has id 0
    if(disks[i].length === 0 || disks[i][disks[i].length-1] < disks[selected][disks[selected].length-1]){
      //perform move
      disks[i].push(disks[selected].pop());
      move++;
      // I guess this is where it goes wrong, but I can't see why
      this.setState({
        history: history.concat([disks]),
        disks: disks,
        move: move
      });
    }
    this.setState({
      selected: null
    });
    console.log(this.state.history);
  }

Please note that the game is otherwise working, meaning the disks array is updating properly etc... It's just the update of the history array that goes wrong somehow. I tried putting disks.slice() into the history.concat as it seemed to me that the history is somehow storing references to the disks array, but that didn't help.

Jondev01
  • 35
  • 4
  • I guess it comes from `history: history.concat([disks]),` which is probably not what you want. Note that your code would be much clearer if you used the `this.setState(updateFunction)` signature of `setState` and separate the different parts. See https://reactjs.org/docs/react-component.html#setstate – adz5A Sep 04 '18 at 10:50
  • @adz5A What I want is that the disks array should be added as a new slot in the history array, leaving the other slots of the history array unchanged. I thought (and still think) this is what history.concat([disks]) does, but I just can't understand what goes wrong. Thank you for the tip how to clean up the code. – Jondev01 Sep 04 '18 at 11:04
  • Slice is only doing shallow copies of objects. You are mutating the disks during the game and appending the same objects to the history (basically what you do is mutate the disk, and add it to the history) so the history will reflect exactly this. You should try creating new disk objects instead of mutating them – adz5A Sep 04 '18 at 11:10
  • `disks[i].push(disks[selected].pop());` this line mutates the disk. Because it is stored in the history, the history is also mutated. – adz5A Sep 04 '18 at 11:13
  • @adz5A " You should try creating new disk objects instead of mutating them ". I thought this is exactly what the line const disks = this.state.disks.slice(); does? Is the problem that the disks array itself contains arrays, such that disks[i] and this.state.disks[i] refer to the same thing? – Jondev01 Sep 04 '18 at 11:28
  • Exactly. This is what shallow copy is, it only copies the "outer" part of the objects, here the array. What you want is a "deep copy" of these objects. If you are using a "functional style" you may want to look at the rest and spread operators to create new objects from old ones whild retaining some of their properties. In your case you should `const newDisk = [...oldDisk, newValue]` and `const newSelectedDisk = disks[selected].slice(-1)` – adz5A Sep 04 '18 at 11:34
  • @adz5A Yes, that was the problem. Replacing const disks = this.state.disks.slice(); with const disks = [this.state.disks[0].slice(), this.state.disks[1].slice(), this.state.disks[2].slice()]; makes it work. However this does not look very pretty. – Jondev01 Sep 04 '18 at 11:35
  • It does not indeed, this is why you should try to decompose the process of updating the game into several independent pieces which take data and return new data. Those pieces (functions) will be passed to `setState` using the signature I was suggesting earlier. If I rearrange our discussion into an answer would you accept it ? – adz5A Sep 04 '18 at 11:37
  • @adz5A thank you. I will look into your most recent comment as I don't really understand it. I'm coming from c++ and am not yet that proficient in javascript (as you can tell). – Jondev01 Sep 04 '18 at 11:39
  • @adz5A Yes I would – Jondev01 Sep 04 '18 at 11:40

1 Answers1

0

The problem comes from this:

disks[i].push(disks[selected].pop());

This mutates the disk at index i in place and mutates the selected disk. Because you store theses references in the history and keep adding references of these objects to the history what you are observing is your game stabilising.

In order to see a little better into what is going on you could try splitting the handleClick method into several parts.

function getNewState (oldState, selectedIndex) {
  if (oldState.selected === selectedIndex) {
    return oldState;
  }
  if (isLegalMove(oldState, selectedIndex)) {
    return {
      ...oldState,
      selected: selectedIndex, //updated the index
      disks: updateDisk(oldState.disks, selectedIndex),
      move: oldState.move + 1
    };
  }
  return {
    ...oldState,
    selected: null
  };
}

You see that I introduced several functions for the different parts isLegalMove and updateDisk, these are to separate the concerns and allow testing to be easier.

Note regarding the use of Array.prototype.slice: as you noticed it only does a shallow copy of a an array, what this means is if you have a nested object and only do a shallow copy of the outer one and then mutate inside it, the original copy will also be mutated. You may want to create a deepCopy instead.

adz5A
  • 2,012
  • 9
  • 10