This post is a continuation of our previous article, a response to this smelly code.

When last we left off, a majority of our duplicate code was refactored away to the GetKeyFor method. Here’s where our method currently stands.

private int MarkConnectingCellsRecursive(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    //set the current location to be clicked.
    grid.Columns[location.Column][location.Row].CellState = CellStates.Down;

    //we seed the count at one here, because at least the current cell is to be
    //marked.
    int countOfMarkedCells = 1; 

    //look all around our current cell for other cells of the same value.
    string rightKey = GetKeyFor(grid, location.MoveRight(), clickedValue);
    string leftKey = GetKeyFor(grid, location.MoveLeft(), clickedValue);
    string downKey = GetKeyFor(grid, location.MoveDown(), clickedValue);
    string upKey = GetKeyFor(grid, location.MoveUp(), clickedValue);

    //if the keys ARE the same value, look around them for the same value, mark them, and add them
    //to the count
    countOfMarkedCells += IfContainsClickedValueThenMark(rightKey, grid, location.MoveRight(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(rightKey, grid, location.MoveLeft(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(rightKey, grid, location.MoveDown(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(rightKey, grid, location.MoveUp(), clickedValue);

    return countOfMarkedCells;
}

Note there are essentially two blocks of logic repeated four times.  We can reorganize the lines of code, and then the method extraction should be obvious:

private int MarkConnectingCellsRecursive6(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    //set the current location to be clicked.
    grid.GetCell(location).CellState = CellStates.Down;

    //we seed the count at one here, because at least the current cell is to be
    //marked.
    int countOfMarkedCells = 1; 

    //look all around our current cell for other cells of the same value.
    string rightKey = GetKeyFor(grid, location.MoveRight(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(rightKey, grid, location.MoveRight(), clickedValue);
    string leftKey = GetKeyFor(grid, location.MoveLeft(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(leftKey, grid, location.MoveLeft(), clickedValue);
    string downKey = GetKeyFor(grid, location.MoveDown(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(downKey, grid, location.MoveDown(), clickedValue);
    string upKey = GetKeyFor(grid, location.MoveUp(), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(upKey, grid, location.MoveUp(), clickedValue);

    return countOfMarkedCells;
}

See the repetition? Pull it out into a method, and you have the code below. This is a VAST improvement over where we started out at the beginning of this series.

private int MarkAndCountCellsWithValueFrom(IGrid grid, CellValues clickedValue, GridCoordinate potentialLocation)
{
    string key = GetKeyFor(grid, potentialLocation, clickedValue);
    return IfContainsClickedValueThenMark(key, grid, potentialLocation, clickedValue);
}

private int MarkConnectingCellsRecursive(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    //set the current location to be clicked.
    grid.GetCell(location).CellState = CellStates.Down;
    
    int countOfMarkedCells = 1;
    
    //look all around our current cell for other cells of the same value.
    countOfMarkedCells += MarkAndCountCellsWithValueFrom(grid, clickedValue, location.MoveRight());
    countOfMarkedCells += MarkAndCountCellsWithValueFrom(grid, clickedValue, location.MoveLeft());
    countOfMarkedCells += MarkAndCountCellsWithValueFrom(grid, clickedValue, location.MoveUp());
    countOfMarkedCells += MarkAndCountCellsWithValueFrom(grid, clickedValue, location.MoveDown());

    return countOfMarkedCells;
}

There’s still an issue with this code – is it obvious? Look at the name of the method it’s calling. “MarkAndCountCells….:” This violates a fundamental principle methods should do one thing, and do it well – be a Unit of code. The method may be doing two things well, but is still doing two things. More on this to come.