This post is a continuation to our previous article, a response to this smelly code.
When we last left off, we replaced the boolean expressions in the first if blocks with meaningful boolean logic.
Now that we have that, we can re-replace the boolean logic with variables that make sense. The first step to making more concise code is to make more readable code.
private int MarkConnectingCellsRecursive2(IGrid grid, int column, int row, CellValues clickedValue)
{
//set the current location to be clicked.
grid.Columns[column][row].CellState = CellStates.Down;
int count = 1;
string rightKey = String.Empty;
string leftKey = String.Empty;
string downKey = String.Empty;
string upKey = String.Empty;
bool columnIsNotFirstColumn = column > 0;
bool columnIsNotLastColumn = column < grid.Width - 1;
bool rowIsNotFirstRow = row > 0;
bool rowIsNotLastRow = row < grid.Height - 1;
if (columnIsNotLastColumn
&& grid.Columns[column + 1][row].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column + 1][row]))
{
rightKey = CreateCellKey(column + 1, row);
}
if (columnIsNotFirstColumn
&& grid.Columns[column - 1][row].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column - 1][row]))
{
leftKey = CreateCellKey(column - 1, row);
}
if (rowIsNotLastRow
&& grid.Columns[column][row + 1].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column][row + 1]))
{
downKey = CreateCellKey(column, row + 1);
}
if (rowIsNotFirstRow
&& grid.Columns[column][row - 1].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column][row - 1]))
{
upKey = CreateCellKey(column, row - 1);
}
if (rightKey != String.Empty && !_searchedCells.Contains(rightKey))
{
//if the one to the right is a match and we haven't looked around it yet,
_searchedCells.Add(rightKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column + 1, row, clickedValue);
}
if (leftKey != String.Empty && !_searchedCells.Contains(leftKey))
{
//if the one to the left is a match
_searchedCells.Add(leftKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column - 1, row, clickedValue);
}
if (downKey != String.Empty && !_searchedCells.Contains(downKey))
{
//if the one below is a match
_searchedCells.Add(downKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column, row + 1, clickedValue);
}
if (upKey != String.Empty && !_searchedCells.Contains(upKey))
{
//if the one above is a match
_searchedCells.Add(upKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column, row - 1, clickedValue);
}
return count;
}
As you can see here, we now are checking to see of the row is not the last row, something that makes considerably more sense.
From here, we can actually start to do real refactoring. Let’s take the first if blocks, they’ve got much repeated logic, but there are four distinct checks here… the first step is to extract them out to methods. The result is below:
private static string GetRightKey(IGrid grid, int column, int row, CellValues clickedValue)
{
string rightKey = String.Empty;
bool columnIsNotLastColumn = column < grid.Width - 1; ;
if (columnIsNotLastColumn
&& grid.Columns[column + 1][row].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column + 1][row]))
{
rightKey = CreateCellKey(column + 1, row);
}
return rightKey;
}
private static string GetLeftKey(IGrid grid, int column, int row, CellValues clickedValue)
{
string leftKey = String.Empty;
bool columnIsNotFirstColumn = column > 0;
if (columnIsNotFirstColumn
&& grid.Columns[column - 1][row].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column - 1][row]))
{
leftKey = CreateCellKey(column - 1, row);
}
return leftKey;
}
private static string GetDownKey(IGrid grid, int column, int row, CellValues clickedValue)
{
string downKey = String.Empty;
bool rowIsNotLastRow = row < grid.Height - 1;
if (rowIsNotLastRow
&& grid.Columns[column][row + 1].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column][row + 1]))
{
downKey = CreateCellKey(column, row + 1);
}
return downKey;
}
private static string GetUpKey(IGrid grid, int column, int row, CellValues clickedValue)
{
string upKey = String.Empty;
bool rowIsNotFirstRow = row > 0;
if (rowIsNotFirstRow
&& grid.Columns[column][row - 1].CellValue == clickedValue
&& IsCellInMarkableState(grid.Columns[column][row - 1]))
{
upKey = CreateCellKey(column, row - 1);
}
return upKey;
}
Now that we have these, adjust the calls to point to our newly extracted methods. Note that if you see duplication above, you’re on the right track; we’ll get back to consolidating these in a future article in this series. Below is the resulting refactored method. Note how much shorter it is now!
private int MarkConnectingCellsRecursive3(IGrid grid, int column, int row, CellValues clickedValue)
{
//set the current location to be clicked.
grid.Columns[column][row].CellState = CellStates.Down;
int count = 1;
string rightKey = GetRightKey(grid, column, row, clickedValue);
string leftKey = GetLeftKey(grid, column, row, clickedValue);
string downKey = GetDownKey(grid, column, row, clickedValue);
string upKey = GetUpKey(grid, column, row, clickedValue);
if (rightKey != String.Empty && !_searchedCells.Contains(rightKey))
{
//if the one to the right is a match and we haven't looked around it yet,
_searchedCells.Add(rightKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column + 1, row, clickedValue);
}
if (leftKey != String.Empty && !_searchedCells.Contains(leftKey))
{
//if the one to the left is a match
_searchedCells.Add(leftKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column - 1, row, clickedValue);
}
if (downKey != String.Empty && !_searchedCells.Contains(downKey))
{
//if the one below is a match
_searchedCells.Add(downKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column, row + 1, clickedValue);
}
if (upKey != String.Empty && !_searchedCells.Contains(upKey))
{
//if the one above is a match
_searchedCells.Add(upKey, String.Empty);
count += MarkConnectingCellsRecursive(grid, column, row - 1, clickedValue);
}
return count;
}