0

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?

2
  • 1
    Seems like CodeReview is more suitable place for this questions.
    – Fabio
    Apr 24, 2017 at 3:35
  • @Fabio we are allowed to look at code here. Code Review tends to be for open ended reviews. This question is nicely focused on one concern. Apr 24, 2017 at 4:01

3 Answers 3

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.

8
  • Thanks for your answer, but I have a question. In the page 35 of Clean Code at Blocks and Indenting, Uncle Bob say your fuctions should be short as possible and it implies if, else and while blocks should be 1 line long using a good descriptive function to tell the intention of the block. Is it really necessary in this case and if yes, which name should I give to that function? Apr 24, 2017 at 3:32
  • @kewersonhugo it's only necessary if you mean to follow that style. It isn't an issue of level of abstraction as turning a nameless abstraction into a named abstraction. You can do that. It is a style that demands many names of you. Off the top of my head successTurn() and failTurn() come to mind. So do validMove() and invalidMove(). But as you build the rest of the code expect to need to reexamine your names as you discover your real needs. Apr 24, 2017 at 3:51
  • 1
    I am pretty sure Uncle Bob would not "push drawBoard down into makeMoveInPosition", he would probably keep these two things separated and make a new function which calls both in sequence (your move function` could be this function). I would also expect him to group the call sequence move, verifyWinner and changePlayer into a new function, just not sure how to name it, something like executeMoveTurn maybe.
    – Doc Brown
    Apr 24, 2017 at 5:39
  • But, @DocBrown, updating the board state isnt part of accomplishing a move? The drawBoard will just present the new state from the board. And I like the idea of executeMoveTurn to keep the if block small. It feels more readable. Apr 24, 2017 at 18:20
  • @kewersonhugo: where did I write "updating the board state isnt part of accomplishing a move"? I said "makeMoveInPosition" and "drawBoard" might be grouped together by a function which calls both in sequence (opposed to "makeMoveInPosition is mixed up with drawBoard", as this answer suggests).
    – Doc Brown
    Apr 24, 2017 at 18:30
3

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);
3
  • I wouldn't recommend changing it to call boardIsEmpty or showInvalidMoveError. Your "doing one thing" code is better - especially as you renamed boardIsEmpty to isValidMove.
    – user253751
    Apr 24, 2017 at 1:58
  • Invalid move is part of the execution control. Changing it to throwing exceptions force you to catch that exception. Which makes you using exceptions as control flow.
    – Fabio
    Apr 24, 2017 at 3:43
  • Thanks for your answer, @immibis. I considered throwing a expcetion in the else block too, but as stated by @fabio, displaying the invalid move to users is part of the application normal flow. Apr 24, 2017 at 18:08
1

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);

Your Answer

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

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