Sunday, March 04, 2007

Save Your Fingers - Let Your Snippets Do The Typing

Snippets are a really cool tool for creating commonly used code blocks or structures that can't be re-used through class libraries/sub-procedures etc. because the definition or layout is similar but not the content of the code. Generally snippets can save you a whole lot of typing. If you are not familiar with Visual Studio 2005 snippets, we suggest you Google them for more information.

I've created some snippets for code I commonly write and posted them in zip file that can be downloaded from Yortsoft website.

These snippets include;
  • dispobj - Expands to create a class definition for an object that correctly implements the dispoable pattern, including the IDisposable interface, suppression of finalisation if disposed prior to the finaliser being called by the GC, use of isDisposing in a common private method to allow only non-managed resources to be disposed when called from the finaliser and an IsDisposed property.
  • propctlapp - Creates an 'appearance property' for use in a (user or custom) control class. This property comes with XML documentation, the category attribute applied with a setting of 'Appearance', the set accessor contains an if condition for checking to see if the value actually changed before calling the Invalidate method (and any other user code you insert), the description attribute (value set by snippet variable) and the browesable attribute set to true.
  • eventdec - Declares an event using the generic EventHandler delegate and creates a protected virtual OnEventName for raising the event. Also includes XML documentation for the event and method created.
  • invokeself - Creates a method, for use within a form or control class, which invokes itself if this.InvokeRequired is true, or executes the required code if this.InvokeRequired is false.
  • memberregions - Creates the following 'regions' in the current code module; Fields, Events, Constructors/Destructors, Public Methods, Public Properties, Overrides, Private Methods, Event Handlers. This is really a personal preference I have for formatting my own code, but I've seen other people on the 'Net doing the same so I thought someone else might like this snippet too.
  • propc - Creates a property with both get and set accessors, with an if (oldvalue != value) condition in the set accessor. Also includes XML documentation for the property created.

I'll add more snippets as I think of them. Let me know what you think of these ones, or if you have an improvement to suggest.

Friday, March 02, 2007

Exceptions You Should Always Break On

One of the great features for debugging in Visual Studio 2005 is the exceptions dialog. This can be accessed from the debug -> exceptions menu. If you don't have this menu item (some default settings picked during installation don't include this option) then you might need to 'customise' your tools bars to add it.

The exceptions dialog allows you to specify how the debugger should behave when an exception is thrown. The most common thing to do here is to specify 'always break on xxx' where xxx is a type or category of exceptions. This is similar to the VB6 'Break On All Errors' option, except that you can specify specific exception types to break on instead of all or nothing. Note : If you want the equivalent of VB6's 'Break On All Errors' option, simply tick the check box next to Common Language Runtime Exceptions.

I regularly use this feature when debugging, as often I know an error is occurring somewhere in the code when I perform a particular action, but one of my event handlers, or my global error handling is preventing me from finding the specific location. By saying to break on all errors, or the specific error I'm dealing with, Visual Studio will stop and break into the code, highlight the line that threw the error, as each error occurs. This lets me quickly and easily locate the source of the problem.

This had lead me to some new thinking on exceptions. I am coming to believe there are some exceptions that should never, ever occur. Of course, we all make mistakes, so that is to say there are some exceptions that should only occur when there is truly a bug.

So far I have only applied this thinking to one exception type (although it may apply to others) - System.NullReferenceException.

System.NullReferenceException is the same as the old VB6 'Object reference not set' error. It basically means you tried to use a property or method on a variable that has not been assigned an object value. Now let's think about the times this might happen;

  1. You simply didn't foresee this variable ever being null (shame on you), and this is a bug. In this case, fix your code so it checks whether the variable is null or not. If it is, and that should be exception, see point 4.
  2. Someone passed you a null argument and you don't accept nulls - in which case you should check the argument before using it and throw the System.ArgumentNullException if it is null. Don't forget to provide the name of the argument.
  3. You're using an object you think might be null sometimes, and if you get the null reference exception you catch it and do something else. BAD BAD BAD - throwing exceptions is bad for performance, and unnecessary try/catch blocks look ugly. Instead, use an if to check if the value is null and do one thing, else do the other.
  4. You're using an object that shouldn't be null, but if it is the problem should throw an exception. Cool, fine, great... but don't throw System.NullReferenceException - it doesn't tell you anything ! Instead, throw a more meaningful exception, even if it means creating one yourself. Say you've attempted to find a plug-in object by name in a collection of loaded plug-ins and the result is a null reference, and that should be an error condition. How about creating and throwing a ConfigurationException instead ? At least this way you, the user, their administrator or their support agent has a better chance of figuring out the problem is a missing plug-in if the error occurs in a deployed system. Also, and possibly more importantly, code calling yours can catch ConfigurationException and confidently handle it if necessary, where as catching System.NullReferenceException is kind of ambiguous - did you get it because the add-in reference was null, or some other variable in the same routine ?

If you take this approach then you can permanently check the 'break on thrown' checkbox for System.NullReferenceException. What does this do for you ? It ensures that if you're debugging the exception occurs you will always know where it happened, and you will always know that it's a problem you need to fix. It also ensures you and your colleagues don't write sloppy code that throws too many exceptions (and catches them) causing poor performance.

So, what do you think ? Should System.NullReferenceException be banned from deliberate or expected exception lists ? Are there any other exception types you think belong here ? Let me know !

Wednesday, February 28, 2007

Creating and Raising Events - The Right Way

Editors Note : There is a follow up post to this one with critical information. If you read this article, please read the follow up as well.

I've seen quite a few people new to C# who haven't grasped the correct/best way to declare and raise events, particularly when writing objects you expect other developers to inherit from.

The most common mistake I see made is pre-fixing the word On to event names, i.e OnProgressChanged.

This is a no-no because because every event (at least in a class that may be inherited from) should have a protected method with that name (i.e OnEventName). For example, if you wanted to have an event that occurred when progress of a long-running operation changed, you'd declare the following (or something like it);

public event EventHandler<ProgressChangedEventArgs> ProgressChanged;

protected virtual OnProgressChanged(ProgressChangedEventArgs e)
{
if (ProgressChanged != null)
ProgressChanged(e);
}

Editors Note : The above sample code has a race condition in it. See the follow up post for the correct way to do this.

The reason for the protected On method is to allow class deriving from this one to raise the event. In .NET you can only raise an event from within the class that declared it, so if you tried to say

if (ProgressChanged != null)
ProgressChanged(e);

in a derived class, you would receive a compile error. The reason for this is that good design dictates only the object that owns the event should know when to raise it, and so only it should be able to. However, since code in derived classes are really part of the object they should also be allowed this privilege. By providing this method to raise the event, and making it protected, we ensure that only the root class, and any derived classes but no external code, can raise the event.

The virtual keyword in the On method declaration is not required, but I prefer to use it as it allows derived classes to know when the event is about to be raised and either prevent it, alter the event arguments, or raise other events/execute other code before or after the event is raised.

Another, less common problem I've seen is code that fails to check if the event delegate is null before invoking it. In .NET you raise an event by invoking a multi-cast delegate, which is a fancy way of saying you just write the event name, i.e

DisplayProgress(e);

will invoke the DisplayProgress event - BUT- if there is no one connected to the event yet a null reference exception will occur, which is why you should always put this first;

if (EventName != null)

Even if you wanted an exception thrown when no one was connected to your event, I would suggest throwing a custom exception with a more meaningful message, rather than allowing the generic null reference exception to be thrown.

Finally, I often see code that explicitly defines a delegate for each event. For example;

public delegate void MyEventDelegate(object sender, MyEventArgs e);

public event MyEventDelegate MyEvent;

This is fine, and there is nothing inherently wrong with it, but Microsoft have (thankfully) provided us with a shortcut, using a generic delegate for all events. The above code can be simplified to;

public event EventHandler<MyEventArgs> MyEvent;

This is not only less typing, but means there's fewer declarations floating around polluting your intellisense/namespaces.

One last thing, some times I've seen code that declares events using delegates that don't conform to normal event handler signatures, i.e

public delegate void MyEventDelegate(int myValue);
public event MyEventDelegate MyEvent;

The problem here is the delegate used doesn't have a sender parameter (declared as an object), nor does it use an object inheriting from EventArgs to provide the myValue integer value to the event handlers.

This is bad practice. All events in the .NET framework and associated libraries (or at least all the ones I've seen) conform to the object sender, eventargs e syntax and it's going to be expected from most people coding against your events. Also, by not using an object inheriting from the EventArgs class to pass your event parameters you're going to have to do a lot more refactoring if you want to add or remove parameters (and any third party code written against the old signature will need to refactoring too). Finally, you haven't provided a reference to the object raising the event, so if someone has a single event handler receiving event notifications from multiple sources, they'll have no idea where the event came from.

The only time it *might* be acceptable to break from this standard is when you don't want to pass any parameters at all, and are 100% sure you never will. In which case it might be ok to pass only a reference to the sender and no EventArgs. I must admit that having to create/receive an instance EventArgs always seems fairly pointless and stupid to me, but the benefit is (again) that you don't have to refactor so much if you add parameters later.

Invoke This !

Often when you're multi-threading you'll want to invoke back to another thread (usually the main thread in your program). This isn't very hard to do, and often leads to code like this;

this.Invoke(new DisplayProgressDelegate(this.DisplayProgress), percentageComplete);

This is fine, but there are a few occasions when this is annoying;

1. Sometimes you may not be sure if you were called from another thread or not. Perhaps you are sometimes called by your own code and sometimes called from an event or via a callback that may or may not be on a background thread. If you're not on another thread, you may not want the overhead of the delegate creation/invoke call.

2. It's a lot more typing that just

DisplayProgress(percentageComplete);

3. It looks untidy, especially if repeated often in your code.

4. You may be about to call someone else's code, either through a call back or an event, and you are unsure if they are multi-threading aware, i.e you're about to raise an event from a background thread and untrusted 3rd party plug-ins may or may not try to access user-interface objects in their event handlers and crash as a result. You'd like protect yourself from this and avoid the problem even if the 3rd party coders didn't do the right thing.

The good news is; there is a fiendishly simple way to overcome these issues. Let's say you want to be able to call a method called DisplayProgress and have it invoked only if needed. All you need to do is alter your DisplayProgress routine so it looks like so;

public void DisplayProgress(int percentageComplete)
{
if (myMainForm.InvokeRequired)
myMainForm.Invoke(new DisplayProgressDelegate(this.DisplayProgress), percentageComplete);
else
{
//TODO: Put your normal code for this procedure here.
}
}

and then you can call it from anywhere, like normal;

DisplayProgress(50);

Ok, let's examine what's going on here. The first line in the DisplayProgress method checks to see if you're on the right thread by calling InvokeRequired on a form (which was presumably created on your user interface thread).

If you're on the same thread as the form and no invoke is required, the else condition is executed and no invoke occurs.

If you're on a different thread, the function invokes itself (recursion is cool :) ). When the method enters itself for the second time, it's on the right thread so the else condition executes. When the code is finished the function returns to it's previous call (the invoke), which has no code following it because all the work is done in the else condition, so the method simply ends and returns to the calling procedure.

The end result is that it doesn't matter which thread you call the method from, it will always execute on the thread that myMainForm was created on.

This can be used for any method, including OnEventName methods that raise events, ensuring that everyone who receives the event is running on the thread that is safe for user-interface access, protecting newbie developers writing plug-ins for your code from the hairy world of multi-threading.

There are a few variations on this syntax. You can, for example, create the delegate pointing to the method as a module level variable and re-use the same instance on each call, which reduces the number of objects created/collected by the .NET framework and gives you a small performance boost and reduces memory pressure if the function may be called many times in quick succession.

One caveat using this code; this code assume that you only ever want to cast to a single thread (the one that myMainForm was created on in this example). If you wanted to cast to different threads then you'd need to pass in a form or control object to the method and use it's InvokeRequired/Invoke members.

Wednesday, January 03, 2007

When ActiveControl Is Not The Active Control

I've created a few container controls in the past by inheriting from the ContainerControl class. This allows the control to host other controls as 'children', and you'd think would do all the right focus management and so on, but you'd be wrong. In fact, the Visual Studio help says "You do not typically inherit directly from the ContainerControl class. Form, UserControl and UpDownBase classes inherit from ContainerControl."

I'm not sure why using ContainerControl as a base class isn't the usual course of action, but I have discovered one reason it's not a good idea - the focus management doesn't work. If you create a custom control inheriting from ContainerControl the host form will always report your custom control as the value of the ActiveControl property, instead of the child control that actually has focus.

In order to properly create a container control you need to implement the IContainerControl interface, and implement code to track the active control and activate it when the appropriate interface members are called. Luckily, this code isn't hard;

//Private member to store the active control in.
private Control activeControl;

bool IContainerControl.ActivateControl(Control active)
{
//If we know the active control, then focus it, otherwise focus the first child control.
bool retVal = false;
if (activeControl != null)
{
activeControl.Focus();
retVal = true;
}
else
{
if (this.Controls.Count > 0)
{
this.Controls[0].Focus();
retVal = true;
}
}
return retVal;
}

Control IContainerControl.ActiveControl
{
get
{
//Return the active control, that we're keeping track of.
return activeControl;
}
set
{
//Keep track of the active control ourselves by storing it in a private member, note that
//that we only allow the active control to be set if it is actually a child of ours.
if (this.Contains(value))
activeControl = value;
}
}

That's all you need to make your container control work. Why this isn't done for us in the ContainerControl (or why it doesn't work), I don't know, but at least the fix is simple.