Exceptions During EventHandler Invocation and Allowing No Subscribers

Ever create a middle-tier library and have a ton of EventHandlers / EventArgs on your classes because you think it will be convenient in some use cases that just aren't needed up front?

In my experience, this always winds up as an annoyance on invocation as the end user of the library would need to wire up all those events even if they weren't being used, or risk a NullReferenceException saying "Object reference not set to an instance of an object".

This post shows a quick and easy pattern to use across any application or library to make consuming events completely optional. You can tell your classes "if no one is listening, don't invoke".

Lets start out with a simple console application and a few classes. We have a forest with some trees:

public class Forest
{
 public int TreeCount { get; private set; }
 public Forest(int numberOfTrees)
 {
  TreeCount = numberOfTrees;
 }

 public void RemoveATree()
 {
  TreeCount--;
 }

 public bool HasTrees { get { return TreeCount > 0; } }
}
And a Chopper that knows how to cut down trees in that forest.
public class TreeChopper
{
    private readonly Forest _forest;
    public TreeChopper(Forest aForest)
    {
        _forest = aForest;
    }

    public void ChopATreeDown()
    {
        if (!_forest.HasTrees)
        {
            throw new NoTreesLeftException("There are no trees in the forest to chop down.");
        }

        OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));
        OnTreeChopped.Invoke(this, new TreeChoppingEventArgs("Finished chopping."));
        OnTreeFalling.Invoke(this, new TreeChoppingEventArgs("Tree is starting to fall down."));
        _forest.RemoveATree();
        OnTreeFell.Invoke(this, new TreeChoppingEventArgs("Tree has fallen. There are "+_forest.TreeCount+" trees left in the forest."));
    }

    public EventHandler<TreeChoppingEventArgs> OnTreeChopping;
    public EventHandler<TreeChoppingEventArgs> OnTreeChopped;
    public EventHandler<TreeChoppingEventArgs> OnTreeFalling;
    public EventHandler<TreeChoppingEventArgs> OnTreeFell;

}

Note that there's 4 events for stages of chopping down that tree, and a simple message EventArgs:

public class TreeChoppingEventArgs : EventArgs
{
    public string Message { get; set; }
    public TreeChoppingEventArgs(string message)
    {
        Message = message;
    }
}

public class NoTreesLeftException : ArgumentOutOfRangeException
{
    public NoTreesLeftException(string message) : base(message){}
}

This is all well and good, and if we set up a runner, we can chop down all the trees in a little console app. The chopping:

public static void Run()
{
    //1. set up a forest.
    var forest = new Forest(numberOfTrees:5);

    //2. wire up all events.
    var chopper = new TreeChopper(forest);

    chopper.OnTreeChopping += OnTreeChoppingEvent;
    chopper.OnTreeChopped += OnTreeChoppingEvent;
    chopper.OnTreeFalling += OnTreeChoppingEvent;
    chopper.OnTreeFell += OnTreeChoppingEvent;


    //3. cut down all trees in the forest
    while (forest.HasTrees)
    {
        chopper.ChopATreeDown();
    }
}

private static void OnTreeChoppingEvent(object sender, TreeChoppingEventArgs e)
{
    Console.WriteLine(e.Message);
}

So this displays the following output:

Starting to chop a tree.
Finished chopping.
Tree is starting to fall down.
Tree has fallen. There are 4 trees left in the forest.
Starting to chop a tree.
Finished chopping.
Tree is starting to fall down.
Tree has fallen. There are 3 trees left in the forest.
Starting to chop a tree.
Finished chopping.
Tree is starting to fall down.
Tree has fallen. There are 2 trees left in the forest.
Starting to chop a tree.
Finished chopping.
Tree is starting to fall down.
Tree has fallen. There are 1 trees left in the forest.
Starting to chop a tree.
Finished chopping.
Tree is starting to fall down.
Tree has fallen. There are 0 trees left in the forest.


But that's pretty verbose. Lets say from the outside perspective, we don't care when the tree chopping started... or even finished. The only thing we care about is when that tree fell, so we can drag it away and use the wood. How do we invoke all of the methods, in case they are used, but get protected in case they are not listened to?

The answer lies in how events work. An event handler is simply a way of keeping track of how to call the delegates -- the places that the event truly gets 'handled'.

Let's take a look at the event handler's definition (.NET 4.0 EventHandler<TEventArgs> where TEventArgs:EventArgs )

This is essentially a subscription list. If you've wired up a handler for the event, it gets added to the list (really an Array). If not, the list is empty.

So to make an event that ensures it's only fired when there's someone listening the assumption is that we simply have to do the following:

Before:

OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));

After:

if (OnTreeChopping.GetInvocationList() != null)
{
OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));
}

Yet in practice, you'll find that this still throws the NullReferenceException. Why? Well, the OnTreeChopping object (the EventHandler itself) turns out to be null. So the first reaction is just to do the following. Check to see if the EventHandler is null, then invoke it if it's not.

Before:

OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));

After:

if (OnTreeChopping != null)
{
OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));
}

So now, if we wrap all of our events in this little block, they'll be 'safe', right? Well, lets assume the answer to that is true for the moment. Writing the same thing over and over again would be smelly; we've just made it 4 lines of code (well, 2 + braces) every time we want to invoke an event. Enter a handy extension method:

public static class EventHandlerExtensions
{
    public static void TryInvoke<T>
        (this EventHandler<T> anEvent, object sender, T eventArgs)
        where T:EventArgs
    {
        if (anEvent != null)
        {
            anEvent.Invoke(sender, eventArgs);
        }
    }    
}

So in our calling code:

Before:

OnTreeChopping.Invoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));

After:

OnTreeChopping.TryInvoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));

And if we only care about the tree being done falling, we can comment out the wiring for the other events as so:

//chopper.OnTreeChopping += OnTreeChoppingEvent;
//chopper.OnTreeChopped += OnTreeChoppingEvent;
//chopper.OnTreeFalling += OnTreeChoppingEvent;
chopper.OnTreeFell += OnTreeChoppingEvent;

The invocation as so:

OnTreeChopping.TryInvoke(this, new TreeChoppingEventArgs("Starting to chop a tree."));
OnTreeChopped.TryInvoke(this, new TreeChoppingEventArgs("Finished chopping."));
OnTreeFalling.TryInvoke(this, new TreeChoppingEventArgs("Tree is starting to fall down."));
_forest.RemoveATree();
OnTreeFell.TryInvoke(this, new TreeChoppingEventArgs("Tree has fallen. There are " + _forest.TreeCount + " trees left in the forest."));

And our output looks like so:
Tree has fallen. There are 4 trees left in the forest.
Tree has fallen. There are 3 trees left in the forest.
Tree has fallen. There are 2 trees left in the forest.
Tree has fallen. There are 1 trees left in the forest.
Tree has fallen. There are 0 trees left in the forest.

 

It's all nice and easy; no messages about chopping, which I don't care about. If the event never gets wired, it doesn't get called. Great. But that begs the question of why to even provide GetInvocationList anyway. Is it useful to us at all? Well, it turns out that there are cases where a misbehaving (read: exception throwing) event handler can spoil invocation for the rest of the handlers.


Chris Tacke had a great writeup on this here (http://blog.opennetcf.com/ctacke/2010/05/27/WhyYouShouldUseEventHandlerGetInvocationList.aspx), and I'm going to apply this scenario to our example. Lets modify our code somewhat to create something bad happening. Lets say we have two event handlers. One transmits the message about the tree having fallen to an RSS feed, and another logs it to some sql database. The sql database server just went down, but we still want to publish the live events to external consumers depending on the data for estimating how much wood they could have available for their production line. First of all, our extension method is now going to swallow the Exceptions, and write them nicely to our console output:

public static void TryInvoke<T>(this EventHandler<T> anEvent, object sender, T eventArgs)
    where T:EventArgs
{
    if (anEvent != null)
    {
        try
        {
            anEvent.Invoke(sender, eventArgs);    
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message);
        }
    
        
    }
}
Second, we're going to create a 'good' event handler, and a 'bad' one:
private static void OnTreeChoppingEventLogToSql(object sender, TreeChoppingEventArgs e)
{
    Console.WriteLine(e.Message);
    throw new InvalidDataException("sql database does not exist");
    Console.WriteLine("Logged to sql.");
}

private static void OnTreeChoppingEventLogToRssFeed(object sender, TreeChoppingEventArgs e)
{
    Console.WriteLine(e.Message);
    Console.WriteLine("Logged to RSS feed.");
}

And when we wire them up:

chopper.OnTreeFell += OnTreeChoppingEventLogToSql;
chopper.OnTreeFell += OnTreeChoppingEventLogToRssFeed;

So what does the output look like?
Tree has fallen. There are 4 trees left in the forest.
sql database does not exist
Tree has fallen. There are 3 trees left in the forest.
sql database does not exist
Tree has fallen. There are 2 trees left in the forest.
sql database does not exist
Tree has fallen. There are 1 trees left in the forest.
sql database does not exist
Tree has fallen. There are 0 trees left in the forest.
sql database does not exist

Wait... what happened there? What about that critical RSS feed? Well, it turns out that the exception thrown blocked further invocation of other event handlers. Keep in mind that event handlers are invoked in the order that they are added to the invocation list. Also, an exception by default prevents further invocation. So if we want to change that behavior, we'll need to manually call each EventHandler<T>, rather than just let the MulticastDelegate handle it.

So our extension method changes to:

public static void TryInvoke<T>(this EventHandler<T> anEvent, object sender, T eventArgs)
    where T:EventArgs
{
    if (anEvent != null)
    {
        foreach (EventHandler<T> wiredHandler in anEvent.GetInvocationList())
        {
            try
            {
                wiredHandler(sender, eventArgs);
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }
        }
    }
}

Which makes our output work:
Tree has fallen. There are 4 trees left in the forest.
sql database does not exist
Tree has fallen. There are 4 trees left in the forest.
Logged to RSS feed.
Tree has fallen. There are 3 trees left in the forest.
sql database does not exist
Tree has fallen. There are 3 trees left in the forest.
Logged to RSS feed.
Tree has fallen. There are 2 trees left in the forest.
sql database does not exist
Tree has fallen. There are 2 trees left in the forest.
Logged to RSS feed.
Tree has fallen. There are 1 trees left in the forest.
sql database does not exist
Tree has fallen. There are 1 trees left in the forest.
Logged to RSS feed.
Tree has fallen. There are 0 trees left in the forest.
sql database does not exist
Tree has fallen. There are 0 trees left in the forest.
Logged to RSS feed.

Note that this won't really work as we want it to if we need to get our exceptions properly bubbled out after the TryInvoke call. To this end, we can introduce some Exception collecting,

including a wrapper for event errors:

public class EventInvocationException : Exception
{
    public List<Exception> Exceptions { get; set; }
    public string Message { get; set; }
    public EventInvocationException(string message, List<Exception> exceptions)
    {
        Message = message;
        Exceptions = exceptions;
    }
}

And then collect them as they are thrown:

public static void TryInvoke<T>(this EventHandler<T> anEvent, object sender, T eventArgs)
    where T:EventArgs
{
    var bubbledExceptions = new List<Exception>();
    if (anEvent != null)
    {
        foreach (EventHandler<T> wiredHandler in anEvent.GetInvocationList())
        {
            try
            {
                wiredHandler(sender, eventArgs);
            }
            catch (Exception e)
            {
                bubbledExceptions.Add(e);
            }
        }
    }

    if (bubbledExceptions.Count>0)
    {
        throw new EventInvocationException("At least one event handler threw an exception", bubbledExceptions);
    }
}

Now since this will be used in a high-level library, allow the invoker to specify allowed behavior, with a default of allowing all of the exceptions to bubble out:

public static class EventArgsExtensions
{
    public static void TryInvoke<T>(this EventHandler<T> anEvent, object sender, T eventArgs, bool allowMultipleExceptions=true)
        where T:EventArgs
    {
        if (allowMultipleExceptions)
        {
            anEvent.TryInvokeWithMultipleExceptions(sender, eventArgs);
        }
        else
        {
            anEvent.TryInvokeWithOnlyOneExceptionAllowed(sender, eventArgs);
        }
    }

    private static void TryInvokeWithOnlyOneExceptionAllowed<T>(this EventHandler<T> anEvent, object sender, T eventArgs)
        where T:EventArgs
    {
        if (anEvent!=null)
        {
            anEvent.Invoke(sender, eventArgs);
        }
    }

    private static void TryInvokeWithMultipleExceptions<T>(this EventHandler<T> anEvent, object sender, T eventArgs)
        where T:EventArgs
    {
        var bubbledExceptions = new List<Exception>();
        if (anEvent != null)
        {
            foreach (EventHandler<T> wiredHandler in anEvent.GetInvocationList())
            {
                try
                {
                    wiredHandler(sender, eventArgs);
                }
                catch (Exception e)
                {
                    bubbledExceptions.Add(e);
                }
            }
        }

        if (bubbledExceptions.Count>0)
        {
            throw new EventInvocationException("At least one event handler threw an exception", bubbledExceptions);
        }
    }
}

Now we can still call the TryInvoke on our event like before:

OnTreeFell.TryInvoke(this, new TreeChoppingEventArgs("Tree has fallen. There are " + _forest.TreeCount + " trees left in the forest."));

However if we want the first exception to really blow up, we can use the old-style plain invoke:

var allowMultipleExceptions=false;
OnTreeFell.TryInvoke(
this, 
new TreeChoppingEventArgs("Tree has fallen. There are " +         _forest.TreeCount + 
    " trees left in the forest."), 
allowMultipleExceptions);

And the best part is that all you have to do from this point on is to call TryInvoke instead of Invoke whenever you're publishing an event, and import the namespace of your extension method.

Posted on 8/24/2011 8:51:00 PM by Jason Nadal

Permalink | Comments |

Categories: design | codeQuality | software | refactoring | development

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Jetbrains Dottrace, Linq 2 Sql, and the case of the static DataContext

I've been in head-down, job-focused, non-blogging mode for a while now, but this issue has been enough to wake me out of hibernation. I've always been best at learning things on the job, since I tend to run into real-world issues that the examples never hit (happy path, anyone?), and I've had an intermittent memory exception in an app that I've been trying to track down on nights & weekends for a while. It's not severe enough to linger, but it's frequent enough (once a month or two) to nag at me for not fixing it.

Enter Jetbrains Dottrace Memory. I've used it for performance analysis a few times in the past, and discovered it's invaluable at tracking down where your problems are. Seriously, I can't recommend the Jetbrains suite of tools enough. Sometime in the past year or so, they split apart DotTrace performance from DotTrace memory & I'm at about a half-upgrade (DotTrace memory 3.5), with a currently bad integration story with Studio & Resharper 6, but that's a side story.

I figured I would need to run a memory snapshot on a machine that runs my application (x64 machine, .NET 4, 32-bit console application in case your curious), since the network connectivity the application relies on doesn't exist on good-old localhost. (In the back of my mind I'm thinking I should take some of the remote server's connection logs, and mock up the remote side using an interface... I still may go down that path, but for now I need to rule out the TcpStream as a factor in the leak).

So I install DotTrace, and didn't bother putting in my personal product key, since I was hoping to grab the dump and analyze it on my development workstation anyway. I fire it up, start my console app, run the dump, and boom. I get stuck mid-dump, my app crashes, and no snapshot. I'll save hours and days of summary here, but how I figured this out was to put in "Console.ReadLine()"s in my code, doing deeper and deeper dumps until I found the line of code causing the problem.

It turns out that my Linq2Sql code is at fault (or DotTrace is... an argument can be made for either side). I have a repository that on construction stores a reference to the DataContext needed for queries. DotTrace failed to allow a memory dump after the data context was used. Wrapping it in dispose did not resolve the issue. A deeper change resolves the single code issue I had, however I would need to analyze the rest of the code I have that still uses Linq2Sql (I'm in favor of the NHibernate / Fluent NHibernate / Lambda Extensions kit for data access these days, but I still have some retro code lying around). The greater concern here is code changes just to allow tooling to work. Part of me is curious if the memory leaks are in fact due to stale DataContext objects that do not get disposed.

I'd be curious to see disposal patterns for Linq2Sql DataContext objects, however part of me thinks disposal of Linq2Sql altogether is the more prudent option. I had found this article on options for context instantiation (http://blog.stevensanderson.com/2007/11/29/linq-to-sql-the-multi-tier-story/), and my app used one of the options that relied on being run in single-thread mode (names changed to protect the guilty):

public static class DataContextHelper
{
   public static MyRequestModelDataContext RequestContext = 
      new MyRequestModelDataContext(ConnectionStrings.MyConnectionString);
   public static MyResultModelDataContext ResultContext = 
      new MyResultModelDataContext (ConnectionStrings.MyConnectionString);
}

 

Unwittingly, by putting a constructor on my repo that went around the static property's context construction, and instead putting the context instantiation inside the repository itself, even having the context being used doesn't prevent the memory dump. I know that having a static class means one instance of that class, and in this case its properties as well, around in memory. I'm not sure what that open data context does to DotTrace, but clearly the result is a crash while attempting to dump. So at this point I start trying to eliminate DataContextHelper in favor of some more atomic operations, and explicit lifecycle of the DataContext.

I started with the above, and a call to create a context in a private member in the repository, and moved to:

public class MyRepository
{
   private MyRequestModelDataContext _context = 
      new MyRequestModelDataContext(ConnectionStrings.MyConnectionString);
   public MyResponse GetResponse()
   {
      var query = from x in context.MyResponses
                  where x.Property.Equals(false)
                  orderby x.AnotherProperty
                  select x;
      var result =  query.FirstOrDefault();
      return MyEntityConverter.ConvertFrom(result);
   }
}

 

Now the interesting thing is that this alone seems to solve the issue... when MyRepository goes out of scope, the garbage collector would need to handle both the repository instance and its own instance of the DataContext.

The only downside to this would be that the instantiation of a context is by nature expensive, however the typical use case would be to create a single repository instance and use that for all of the data calls within a specific operation set or transaction anyway. So I make the change, and see the behavior of crashing memory dumps; clearly this is not the answer. So lets make things a little more interesting. One more change to the repository to make sure the context is cleaned up:

 

public class MyRepository : IDisposable
{
   ...
   public void Dispose()
   {
      if (_context != null)
      {
         _context.Dispose();
      }
   }
}

 

And then back a layer. The original code was:

var repo = new MyRepository();
return repo.GetResponse();

 

And the changed code is:

MyResponse result;
using (var repo = new MyRepository();
{
   //dump 1
   result = repo.GetResponse();
   //dump 2
}
//dump 3
return result;

 

The dump comments are Console.ReadLine's injected in to track when the dumps fail. While the console application is set up to wait, I can run over to DotTrace's "Control Profiling" window and dump the memory into another snapshot (or try to, anyway). The results of the above change are that the three dump points work fine. The next step of course would be to extend out these changes to any other repositories / calls.  The repositories are easy... the calls are not quite as easy. I find usages of the repository class, and wrap everything in using statements; the next step is verifying at least that the functioning of the application is not impaired, using a test run. So far so good. I then go and run memory dumps at various points throughout the process -- mission succeeded. I'm still unclear as to the nature of the behavior, however the code change allows me to proceed with my diagnostic regiment.

What is a bit unsettling still is that there are points in the application during execution where I am unable to dump out memory without crashing. I need to keep an eye on where there are objects around that depend on open data contexts, and those seem to be directly related to when I am able to successfully generate snapshots.

Posted on 7/21/2011 8:00:00 PM by Jason Nadal

Permalink | Comments |

Categories: development | performance

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Membership Providers and Password Hashing -- be careful!

While recreating some boilerplate code that winds up getting created for every set of apps -- including a membership provider, roles provider, etc, I initially went right for setting hashAlgorithmType based right on the enum.

More info about membership properties here.

This enum only gives three values -- MD5, SHA1, and None. The problem here is that both of those algorithms have been proven broken for some time (hopefully ASP.NET 4.0 will resolve this!). The answer of course is to use something with a little more difficulty to it... say by using SHA512Managed() and a salt. This is just another one of those times when setting values to canned possibles can be a dangerous move. This is especially true with authentication / encryption.

 

Posted on 4/8/2010 6:43:00 AM by Jason Nadal

Permalink | Comments |

Categories: asp.net | development | security

Tags: , ,

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Exception Handling and Language Restriction in C#

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.

Posted on 7/21/2009 8:00:00 AM by Jason Nadal

Permalink | Comments |

Categories: codeQuality | development

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

An Exercise in Refactoring: Part 6

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

Posted on 5/6/2009 7:00:35 AM by Jason Nadal

Permalink | Comments |

Categories: development | codeQuality | refactoring

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

An Exercise in Refactoring: Part 5

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

Posted on 5/5/2009 6:42:40 AM by Jason Nadal

Permalink | Comments |

Categories: development | patterns | refactoring | codeQuality

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

When NOT to Performance Tune Your Application

On a recent project I was told by a colleague about a certain SQL query generated by entity framework, that was ridiculously out of hand. Entity Framework allows you to pretty easily create a simple Data Access to the Table Per (Sub) Type pattern.

What this means is that you may have an inheritance of both a Student and an Instructor, derived from a Person, and query to retrieve a strongly typed object. So here’s where performance & optimization comes in. There’s a couple of ways to query against this data model.

Method 1: Implicit Typing

var query = from p in Persons
            where p.PersonID.Equals(_personID)
            select p;

Method 2: Explicit Typing

var query = from p in Persons.OfType<Instructor>()
            where p.PersonID.Equals(_personID)
            select p;

They seem pretty similar, however there’s quite a significant difference in what gets generated. By using method 2, you wind up letting Entity Framework know exactly what table it’s querying against. Which means your SQL code looks something like this:

SELECT
    PersonID,
    Column1,
    Column2,
    Column3
FROM
    Instructor
WHERE
    PersonID = @PersonID

However when you don’t specify the type, Entity Framework constructs a SQL query intended to make SQL go and figure it out (keep in mind that there’s no automatic discriminator column – it figures out type based off of the primary key – the ID column. More on this in a minute). The generated code looks something like this:

SELECT
    PersonID,
    Column1 as [0x01],
    Column2 as [0x02],
    CASE WHEN [1x01] IS NOT NULL THEN CAST(INT, [1x01])
    ... many more case, casts, for every column in every table ...
    
FROM
    Person
    UNION ALL SELECT 
        PersonID as [1x01],
        Column1 as [1x02],
        Column2 as [1x03]
        UNION ALL SELECT
                ... many more unions for every table ...
WHERE
    PersonID = @PersonID
        

Now you can see that this query gets very complex as a product of:

a) the number of subtypes

b) the number of columns for each type

The query generated gets the Cartesian product of all columns, and looks for the one where the key isn’t null – that’s the “winner” subtype. I imagine (haven’t yet tried this) that having a nested subtype involved here (like BusinessStudent in the linked example above) would cause an even more ugly nesting of the union within another union statement.

Now back to the point of this article – performance. How bad is what we see above? In an empirical example, thanks to JetBrains’ Dottrace and nunit tests I observed averages of:

Method 1: 126ms for the query to run

Method 2: 25ms for the query to run

I had then discovered the benefits of precompiling Entity Framework view code to optimize the SQL generation. This bought me roughly 26% gain in performance for the specific empirical examples.

Method 1: 100ms

Method 2: 20ms

Now we have roughly 80ms to play with – if the code to get from Method 1 to Method 2 (we don’t know the type that we’re retrieving, however we want the optimized query of Method 2) is more than 80ms, then the performance “fix” will be worse than the problem.

So far, given the constraint of EF (for now), and the Table Per (Sub) Type pattern, the only solution that comes to mind is reflection – this would involve a stored type as a discriminator column of sorts, then reflecting on that type, and calling the generic Person.OfType<T>() method via reflection. This costs us an extra query and reflection – neither of which are cheap. A separate empirical example (not the same code as the first) brings the total cost to ~350ms, a net performance loss of 250ms.

Method 1’s performance would have to degrade (through additional columns/subtypes) by ~250ms more in order to justify rolling a custom discriminator and reflecting to grab the subtype.

This was a pretty interesting exercise in when not to make performance optimizations that you know will need to be done long-term.

Posted on 4/30/2009 7:01:00 AM by Jason Nadal

Permalink | Comments |

Categories: development | performance | software

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

VMs @ Home Development

Wow... it's been a while since I've updated this blog.

Today's topic is one that took me a while to get to... not to writing, but to actually implementing. More to the point, it took me an eternity to

be convinced that a) vms for doing work (even for yourself) at home is a Good Thing (tm), b) is not just good, but essential, and c) vmware is a

better product and easier to use than MS Virtual Server and Virtual PC.

Some things it takes me a while to get drilled into my head on -- these are the lessons that are hard learned. I say this as I just exit my own webform post editor in favor of writing my posts in notepad... this is something I constantly harp on my wife for as something you just should not do. Who wants to rewrite a 3 page textbox entry after they've already typed it!

That was actually a poor segue, but it at least serves to illustrate my point... avoid getting burned.

I'm a developer. I like cutting edge stuff.

Those two statements together? Beta Testing for the win.

Over the years (well, since I tricked my way into the win '98 beta back in high school), I've tried countless software that was close-to-but-not-quite ready for prime-time. I've lived without a functional dvd player, lived without sound. Lived without being able to display anything on screen (well... except for BIOS), and headed into it face-first. (Till Windows Home Server!)

With my development environment, I've learned that VMWare is the best way to allow me to try out whatever betas I want on my host OS (currently running Win7 with nary an issue, now that I've told it to ignore the fact that the 64bit drivers are unsigned, and got my hands on some beta drivers for other cards). All I have to do is use some snapshots in VMWare Workstation, and I can revert back in the dev environment to stable points! Now I can have my nightly builds of resharper 4.5, and roll back if I hosed my working environment! Add to this the fact that I can share my USB devices, and now I can synch my iPod when I'm out of the state. (I can also have the VMs net connection go through my cell phone... really cool if I'm on the road)

The other cool things are being able to use Unity to have virtual applications running side-by-side with host windows, and being able to have my native 2560x1600 resolution OS on a virtual machine. For $189 this is invaluable, even though it's a steep price to begin with.

 

 

Posted on 3/13/2009 6:27:00 PM by Jason Nadal

Permalink | Comments |

Categories: development | hardware | software

Tags: , , ,

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Microsoft Cardspace Codename "Geneva"

It seems the cardspace team has been at work simplifying the user experience for Cardspace prompts. See codename "Geneva". This looks pretty good; they've even included the new Geneva server for creating managed cards, which is something I'd like to explore at some point. I'm very curious what work if any has been done to integrate OpenID in conjunction with cardspace.

I'm not sure just how far Cardspace is penetrating, as I've yet to run across a site (aside from Windows Live, and even that's in beta, and has been since at least August of 2007!), which actually uses cardspace for authentication. I have found many articles from the 2006/2007 timeframe purporting firefox 3.0 support of cardspace, however the plugins online don't show them. Windows live login requires IE to even attempt a cardspace login.

Posted on 12/5/2008 8:07:00 PM by Jason Nadal

Permalink | Comments |

Categories: development | network | vista | windows | cardspace | security

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

Javascript scoping woes

I got re-energized in javascript thanks to a great talk from a colleague today.

I came home and elected to try out my new l33t skillz, and promptly hit a roadblock due to the dreaded scoping of "this".

Basically the scenario is this; I have an object MyObject with a function Reminder(), which gets called after a 10 second delay. The reminder pops up, and when the user closes the reminder, the object's ResetTimer() function gets called. The last line of the ResetTimer function that I was trying to do was:

setInterval( this.Reminder, 10000), and the Reminder function acts on "this". The problem is that when the Reminder function gets called, "this" becomes window. The easy fix, as outlined in this great blog post by Alex Le is to use a closure function, passing the instance into the closure:

setInterval(function(that) { that.Reminder(); }, 10000, this);

The last step was to refactor up to the prototype and all was good!

Posted on 11/14/2008 9:05:00 PM by Jason Nadal

Permalink | Comments |

Categories: development | javaScript

Tags:

Be the first to rate this post

  • Currently 0/5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5