Return to Answer

generateRandomCell()

create()

This code should be moved to constructor basically.

generateRandomCell()

printBoard()

For simple text formatting use String.format(...) or System.out.printf(...): no need to count spaces manually ;-)

• use BOARD_SIZE^2 instead of 16.
• using the same random value both for choosing the cell and deciding whether put there 2 or 4 determines which cells may get 2 and which 4 when chosen.
• instead of (int) (Math.random() * count) it's probably better to do have an instance of Random and do random.nextInt(count).

• use BOARD_SIZE*BOARD_SIZE instead of 16.
• using the same random value both for choosing the cell and deciding whether put there 2 or 4 determines which cells may get 2 and which 4 when chosen.
• instead of (int) (Math.random() * count) it's probably better to do have an instance of Random and do random.nextInt(count).

This is really ugly and misleading. I started reading main code first and it really made me scratch my head for few minutes wondering how isLegal() and move() work without providing any actual move data :? Only after a while I noticed that it so happens that setDirection(userMove) is always called upfront sets a "temporary state" for them... setDirection(userMove) should not exist (or at least not be accessible) and both isLegal() and move() should have a userMove param.
As a first fix, setDirection(userMove) may be made private and called at the beginning of isLegal(userMove) and move(userMove). It's still ugly though, as it uses a "temporary" instance state instead of local vars. To fix this, you can introduce a simple static inner class called for example DirectionConfig with these 6 fields (rowStart, columnStart, rowStep, columnStep, nextRow, nextColumn) and refactor setDirection(userMove) to return a new instance of this class, that move(userMove) and isLegal(userMove) would use instead of Game's instance fields (in such case you can also rename setDirection(userMove) to getDirectionConfig(userMove)).

• it is not used outside of Game, so it should rather be an inner class (static).
• it serves both as a holder of the board state and as a holder of a temporary flag hasMerged used in Game.move(). Combining these 2 purposes is really ugly and confusing. At the very least, the meaning of this flag should be documented.

This is really ugly and misleading. I started reading main code first and it really made me scratch my head for few minutes wondering how isLegal() and move() work without providing any actual move data :? Only after a while I noticed that it so happens that setDirection(userMove) is always called upfront sets a "temporary state" for them... setDirection(userMove) should not exist (or at least not be accessible) and both isLegal() and move() should have a userMove param.
As a first fix, setDirection(userMove) may be made private and called at the beginning of isLegal(userMove) and move(userMove). It's still ugly though, as it uses a "temporary" instance state instead of local vars. To fix this, you can introduce a simple static inner class called for example DirectionConfig with these 6 fields (rowStart, columnStart, rowStep, columnStep, nextRow, nextColumn) and change setDirection(userMove) to return a new instance of this class, that move(userMove) and isLegal(userMove) would use instead of Game's instance fields (in such case you can also rename setDirection(userMove) to getDirectionConfig(userMove)).

• it is just a tiny helper that will never be used outside of Game, so it should rather be an inner class (static).
• it serves both as a holder of the board state and as a holder of a temporary flag hasMerged used in Game.move(). Combining these 2 purposes is really ugly and confusing. At the very least, the meaning of this flag should be documented.