Does this code have only one level of abstraction?

Question

I'm trying to apply some good practices from Clean Code in my code, but I'm stuck trying to figure out if my code has a [G34] code smell, which tells that functions should descend only one level of abstraction.

var tryToMakeAMove = function(line, column) {
  if( board[line][column] === EMPTY) {
    makeMoveInPosition(line, column);
    drawBoard();
    verifyWinner();
    changePlayer();
  }
  else {
    changeStatusMessage("Invalid Move!");
  }
}

I'm doing too much in this function or it is okay to call all those functions in the if block? If I'm doing too much, how could I refactor this function to do only one thing?

Answer 1

I'm a student of Clean Code myself. Here's my attempt at Uncle Bob's style.

var takeTurn = function(line, column) {
  if( moveIsValid(line, column) ) {
    move(line, column);
    verifyWinner();
    changePlayer();
  }
  else {
    changeStatusMessage("Invalid Move!");
  }
}

First we must decide what level of abstraction we're working at. Most of the work here isn't about the move but basic turn taking. The if condition ties us to details of move rules and not turn taking so I've abstracted that away with a function.

That free's this function of other move concerns that might need to be added like a check against moving a queen like knight, a check if the user is illegally attempting to capture their own pieces, diagonally moving a pawn that isn't capturing, castling after the king has moved, etc.

Is it now at one level of abstraction? Well updating the board after a move is not about turn taking. So I've pushed drawBoard() down into makeMoveInPosition()

verifyWinner() and changePlayer() are about turn taking. For instance what if you decide the consequence for making an invalid move is loosing a turn or loosing the the game? Chess is sometimes played that way. So I think it's fine to have them here where they can be moved around at will.

I've also renamed the function so that anyone looking inside the function for the first time wont be surprised that it's full of turn taking logic and not move logic. That follows something called the principle of least astonishment. It's nice when you look in a function and it does pretty much what you expected. The best way to foster that is give the function a good name and be faithful to it's meaning.

Answer 2

Level of abstraction

I would consider the if statement in line 1

if( board[line][column] === EMPTY) 

and the changeStatusMessage() call

changeStatusmessage("Invalid Move!")

as a lower level of abstraction then the other ones. The other ones are rather abstract, there are no implementation details visible. But the given two lines are pretty low level. My advice for the first refactoring would be

if( boardIsEmpty(line, column) )

and

showInvalidMoveError()

Doing one thing

As for how much you are doing I indeed would say you are doing too much. You check for an empty board and then execute a move or else show an error. The execution however is split into several steps, which in this sense could also be a different kind of abstraction. So consider todo another refactoring:

if( isValidMove(line, column) )
    executeMove(line, column);
else
    showInvalidMoveError();

One might even argue that it is still to much doing, as you are also handling an error.

So I would maybe just through an error like this

else throw Error(INVALID_MOVE);

Answer 3

Based on the names of the functions I believe the functions

drawBoard();
verifyWinner();
changePlayer();

are already at a different abstraction layer than tryToMakeAMove() which seems like a much lower level, yet also appears to be a mix of responsibilities.

If your program is asking for input in a loop until a valid move is provided, then that whole process should be it's own function existing at the same level as the functions I mentioned. That process can be organized into:

1. Collect player input
2. Validate the input
3. If invalid/show message, else perform the move
4. Repeat until there's a valid move

function move(player, board) {
  let moveIsValid == false;
  while(!moveIsValid) {
    const {line, column} = collectUserInput(player);
    const moveIsValid = isMoveValid(line, column, board);
    if(!moveIsValid) {
      changeStatusMessage("Invalid Move!");
    } else {
      updateBoardWithMove(line, column, board);
    }
  }
}

And isMoveValid could be:

function(line, column, board) {
 return board[line][column] === EMPTY
}

And finally the top level would look like this:

let currentPlayer = 'Player1';
let winner = undefined;
const board = new Board();
while (!winner) {
  move(currentPlayer, board);
  drawBoard(board);
  winner = verifyWinner(board);
  currentPlayer = changePlayer(currentPlayer);
}
celebrate(winner);