Restless C#ding

In this post, by Eric Lippert, he goes through part 4 of his iterator block series. What's interesting in this post is the statement on how yield statements aren't allowed in "catch" blocks. The interesting part of the article is not just some of the mindset of proposing MSIL for how this could actually be accomplished, but really, the part of the post that matters most is here:

And really, what’s the usage case that motivates this situation in the first place? Do people really want to try something and then yield a bunch of results if it fails?

The crux of the arguement is not at all that the feature is missing, but that the mindset of attempting to do so is abusing exceptions, and hurting the performance of the code you write. Exceptions should be exceptional situations. They should not occur during "normal" behavior (the so-called happy path). I believe Hunt & Thomas stated it best:

We believe that exceptions should rarely be used as part of a program's normal flow; exceptions should be reserved for unexpected events. Assume that an uncaught exception will terminate your program and ask yourself, "Will this code still run if I remove all the exception handlers?" If the answer is "no," then maybe exceptions are being used in nonexceptional circumstances.

To bring the point home, take a look at this information compiled by Roger Orr on how exceptions affect performance. Some examples should really stand out.

Jeff Atwood of Coding Horror (can't recommend his blog enough!) goes a step further, showing how knowledge of some internal .NET functions which rely on exception handling to provide their magic can indirectly degrade your performance as well.

Lippert's post was interesting, but I felt the case should be made more strongly to just give a second or third look on when exceptions are used.

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

When we left off yesterday, I mentioned that the rest of the day’s work on this refactoring (I do have a day job as well, please keep in mind!) I’d spend replacing the Grid.Columns[column][row]:Cell. Well in order to do that, I’ve introduced a new method in the IGrid interface:

public Cell GetCell(GridCoordinate coordinate)
{
    return Columns[coordinate.Column]
        [coordinate.Row];
}

This allows us to do some interesting things in the long term. But really it just makes parameter lists that are passed into methods cleaner.

Now we can get to the fun part – all those GetXKey methods… one for each direction. There’s so much duplication there! See if you can find the similarities in purpose and form between two of the four methods.

private static string GetRightKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{

    string rightKey = String.Empty;
    bool columnIsNotLastColumn = location.Column < grid.Width - 1; ;
    if (columnIsNotLastColumn
        && grid.Columns[location.Column + 1][location.Row].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column + 1][location.Row]))
    {
        rightKey = location.MoveRight().CreateCellKey();
    }
    return rightKey;
}

private static string GetLeftKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    string leftKey = String.Empty;
    bool columnIsNotFirstColumn = location.Column > 0;
    if (columnIsNotFirstColumn
        && grid.Columns[location.Column - 1][location.Row].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column - 1][location.Row]))
    {
        leftKey = location.MoveLeft().CreateCellKey();
    }
    return leftKey;
}

Any ideas? The problem is that there’s a lot of different code there. They’re looking at different criteria in that first boolean expression, but that logic is clear – is the cell (location) that’s being passed in outside the bounds of the Grid. If the cells a legitimate location, and meets the other criteria, then create the cell key.

So what we can introduce is a GridCoordinateBoundary class, representing the measurable borders (boundaries) of our Grid object.

public class GridCoordinateBoundary
{
    public int MinimumRow { get; set; }
    public int MaximumRow { get; set; }
    public int MinimumColumn { get; set; }
    public int MaximumColumn { get; set; }

    private GridCoordinateBoundary(int minimumRow, int maximumRow, int minimumColumn, int maximumColumn)
    {
        MinimumRow = minimumRow;
        MaximumRow = maximumRow;
        MinimumColumn = minimumColumn;
        MaximumColumn = maximumColumn;
    }

    public static GridCoordinateBoundary CreateFrom(int minimumRow, int maximumRow, int minimumColumn, int maximumColumn)
    {
        return new GridCoordinateBoundary(minimumRow, maximumRow, minimumColumn, maximumColumn);
    }
    
    public bool IsWithinBoundary(GridCoordinate location)
    {
        return (location.Row >= MinimumRow)
               && (location.Row < MaximumRow)
               && (location.Column >= MinimumColumn)
               && (location.Column < MaximumColumn);
    }
}

Now we can start pulling the logic together. We don’t need the 4 unique GetDownKey, GetUpKey, etc.

private static string GetKeyFor1(IGrid grid, GridCoordinate nextLocation, CellValues clickedValue)
{
    string key = String.Empty;
    if (grid.GetBoundary().
            IsWithinBoundary(nextLocation)
        && grid.GetCell(nextLocation).CellValue 
            == clickedValue
        && IsCellInMarkableState(
            grid.GetCell(nextLocation)))
    {
        key = nextLocation.CreateCellKey();
    }
    return key;
}

Note that the logic is right in the if block. Based on literature I’m currently reading (I’d highly recommend Robert C. Martin’s Clean Code so far), pulling the boolean expressions out for variable names is cleaner. This gives us the following result at the end of a short day’s work. I will spend the rest of the day getting the GetCell() method propagated to replace the Column[][] syntax that’s pervasive in the solution.

private static string GetKeyFor(IGrid grid, GridCoordinate nextLocation, CellValues clickedValue)
{
    string key = String.Empty;
    bool locationIsWithinGridBoundary = grid.GetBoundary().IsWithinBoundary(nextLocation);
    bool cellValueIsSameAsClicked = grid.GetCell(nextLocation).CellValue == clickedValue;

    if (locationIsWithinGridBoundary
        && cellValueIsSameAsClicked
        && IsCellInMarkableState(grid.GetCell(nextLocation)))
    {
        key = nextLocation.CreateCellKey();
    }
    return key;
}

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

Remember those columns and rows everywhere, and how tough it was to swap them everywhere? Well this is a prime time to fix that. How else can we reduce parameter count, while still passing in the same information? Enter complex object.

In this case, we can gain:

• Reduction of column / row parameters to just location
• Give the CreateCellKey(x,y) method a home (really, it gives a key for a particular coordinate.

So our first step is to create the new object and move in the CreateCellKey method; note this is also implementing a static create method:

public class GridCoordinate
{
    public int Column { get; set;}
    public int Row { get; set; }

    public static string CreateCellKey(GridCoordinate location)
    {
        return location.Column + "," + location.Row;
    }

    public static GridCoordinate Create(int column, int row)
    {
        return new GridCoordinate {Column = column, Row = row};
    }
}

And then we have to fix the calls that just deal with columns and rows. Note the code is longer for now, but we’ll address this with reusability in the near future!

Our resulting changed code is as follows:

private static string GetRightKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{

    string rightKey = String.Empty;
    bool columnIsNotLastColumn = location.Column < grid.Width - 1; ;
    if (columnIsNotLastColumn
        && grid.Columns[location.Column + 1][location.Row].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column + 1][location.Row]))
    {
        rightKey = GridCoordinate.CreateCellKey(GridCoordinate.Create(location.Column + 1, location.Row));
    }
    return rightKey;
}

private static string GetLeftKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    string leftKey = String.Empty;
    bool columnIsNotFirstColumn = location.Column > 0;
    if (columnIsNotFirstColumn
        && grid.Columns[location.Column - 1][location.Row].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column - 1][location.Row]))
    {
        leftKey = GridCoordinate.CreateCellKey(GridCoordinate.Create(location.Column - 1, location.Row));
    }
    return leftKey;
}

private static string GetDownKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    string downKey = String.Empty;
    bool rowIsNotLastRow = location.Row < grid.Height - 1;
    if (rowIsNotLastRow
        && grid.Columns[location.Column][location.Row + 1].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column][location.Row + 1]))
    {
        downKey = GridCoordinate.CreateCellKey(GridCoordinate.Create(location.Column, location.Row + 1));
    }
    return downKey;
}

private static string GetUpKey(IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    string upKey = String.Empty;
    bool rowIsNotFirstRow = location.Row > 0;
    if (rowIsNotFirstRow
        && grid.Columns[location.Column][location.Row- 1].CellValue == clickedValue
        && IsCellInMarkableState(grid.Columns[location.Column][location.Row - 1]))
    {
        upKey = GridCoordinate.CreateCellKey(GridCoordinate.Create(location.Column, location.Row - 1));
    }
    return upKey;
}

private int IfContainsClickedValueThenMark(string key, IGrid grid, GridCoordinate location, CellValues clickedValue)
{
    int countOfMarkedCells = 0;
    if (key != String.Empty && !_searchedCells.Contains(key))
    {
        _searchedCells.Add(key, String.Empty);
        countOfMarkedCells += MarkConnectingCellsRecursive(grid, location, clickedValue);
    }
    return countOfMarkedCells;
}

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 = GetRightKey(grid, location, clickedValue);
    string leftKey = GetLeftKey(grid, location, clickedValue);
    string downKey = GetDownKey(grid, location, clickedValue);
    string upKey = GetUpKey(grid, location, 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, GridCoordinate.Create(location.Column + 1, location.Row), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(leftKey, grid, GridCoordinate.Create(location.Column - 1, location.Row), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(downKey, grid, GridCoordinate.Create(location.Column, location.Row + 1), clickedValue);
    countOfMarkedCells += IfContainsClickedValueThenMark(upKey, grid, GridCoordinate.Create(location.Column, location.Row- 1), clickedValue);

    return countOfMarkedCells;
}

Now that we’ve done that, we can really do some cool things. Since we have a reference to a distinct location as an object, that object knows where it is. You can treat it like a plotter. Given a location, move up, down, left, right.(Note, we’ll probably want to rename this object to something like GridPlotter, so it makes more sense… this would have a GridCoordinate object, but for now, we’ll place the methods right in the GridCoordinate object:

public GridCoordinate MoveUp()
{
    return Create(Column, Row - 1);
}

public GridCoordinate MoveDown()
{
    return Create(Column, Row + 1);
}

public GridCoordinate MoveLeft()
{
    return Create(Column - 1, Row);
}

public GridCoordinate MoveRight()
{
    return Create(Column + 1, Row);
}

public string CreateCellKey()
{
    return CreateCellKey(this);
}

This lets you refactor code like follows. Instead of the +1 and –1 everywhere:

countOfMarkedCells += 
    IfContainsClickedValueThenMark(
    rightKey, grid, 
    GridCoordinate.Create(location.Column + 1, 
        location.Row), 
    clickedValue);

You can just put:

countOfMarkedCells += IfContainsClickedValueThenMark(
    rightKey, grid, 
    location.MoveRight(), 
    clickedValue);

At this point, unit tests all still pass, and app feels pretty good still, so this is a pretty successful refactoring process thus far. For the time until the next post, I’ll be making sure all areas of code referencing this class (the GridController class) no longer use pure “int column, int row” values, but rather use the more robust GridCoordinate class.

After this is complete, the GridCoordinate class will be renamed, and co-located near the Grid itself. Right now, this is only referenced by the GridController, but in reality, the GridCoordinate should serve as the base of all Grid cell lookups. (Rather than always using the Grid.Columns[column][row]:Cell).