An Exercise in Refactoring

Today I spent some time refactoring the worst remaining methods of my SameBlox game (which will be updated on CodePlex once the refactoring is 'complete' for a bit). It's interesting to see what I was thinking a few years back (the last time I had touched this code) -- since I'm primarily a web developer, the fact that this is a win form app gives even more of a chance to show code flaws. If I could go back a few years, and tell myself something (about this code, naturally!), I'd drill "separation of concerns" into my head. I'm just not seeing it anywhere here -- there's game logic in the Form.cs file, there's scoring and game controlling in the Grid (main gameboard class -- who does WAY too much), and the methods were gigantic in some cases, and cryptic and gigantic in others. The code of the day is as follows. Here's one of the (only) remaining large methods left. This will be refactored over a post or two in the coming days; give it a try as an exercise – there’s a lot of similar code here!
private int MarkConnectingCellsRecursive(IGrid grid, int gridX, int gridY, CellValues clickedValue)
{
    //set the current location to be clicked.
    grid.Columns[gridX][gridY].CellState = CellStates.Down;

    int count = 1;

    string rightKey = String.Empty;
    string leftKey = String.Empty;
    string downKey = String.Empty;
    string upKey = String.Empty;


    if (gridX < grid.Width - 1 && gridY >= 0 && gridY <= grid.Height - 1 && grid.Columns[gridX + 1][gridY].CellValue == clickedValue
        && (grid.Columns[gridX + 1][gridY].CellState == CellStates.Normal || grid.Columns[gridX + 1][gridY].CellState == CellStates.Down))
    {
        rightKey = CreateCellKey(gridX + 1, gridY);
    }
    if (gridX > 0 && gridY >= 0 && gridY <= grid.Height - 1 && grid.Columns[gridX - 1][gridY].CellValue == clickedValue
        && (grid.Columns[gridX - 1][gridY].CellState == CellStates.Normal || grid.Columns[gridX - 1][gridY].CellState == CellStates.Down))
    {
        leftKey = CreateCellKey(gridX - 1, gridY);
    }
    if (gridX >= 0 && gridX <= grid.Width - 1 && gridY < grid.Height - 1 && grid.Columns[gridX][gridY + 1].CellValue == clickedValue
        && (grid.Columns[gridX][gridY + 1].CellState == CellStates.Normal || grid.Columns[gridX][gridY + 1].CellState == CellStates.Down))
    {
        downKey = CreateCellKey(gridX, gridY + 1);
    }
    if (gridX >= 0 && gridX <= grid.Width - 1 && gridY > 0 && grid.Columns[gridX][gridY - 1].CellValue == clickedValue
        && (grid.Columns[gridX][gridY - 1].CellState == CellStates.Normal || grid.Columns[gridX][gridY - 1].CellState == CellStates.Down))
    {
        upKey = CreateCellKey(gridX, gridY - 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, gridX + 1, gridY, 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, gridX - 1, gridY, clickedValue);
    }
    if (downKey != String.Empty && !_searchedCells.Contains(downKey))
    {
        //if the one below is a match                
        _searchedCells.Add(downKey, String.Empty);
        count += MarkConnectingCellsRecursive(grid, gridX, gridY + 1, clickedValue);
    }
    if (upKey != String.Empty && !_searchedCells.Contains(upKey))
    {
        //if the one above is a match                
        _searchedCells.Add(upKey, String.Empty);
        count += MarkConnectingCellsRecursive(grid, gridX, gridY - 1, clickedValue);
    }
    return count;
}

Some things to note:

  • CreateCellKey just returns a string in the format “x,y”
  • _searchedCells is a hashtable whose keys are the results of CreateCellKey
  • gridX and gridY mark where the user clicked. All cells touching either the original cell or a marked cell, and are the same cell value as the cell in question are to be marked.

Posted on 5/2/2009 6:31:00 PM by Jason Nadal

Permalink | Comments |

Categories:

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5
blog comments powered by Disqus