Monthly Archives: November 2010

Common mistakes when working with exceptions

Rethrowing and destroying the stacktrace

This is a evil one. Because it’s really easy to use it and the compiler do not complain about it (it should be easy to spot by the compiler). The code also looks correct if you don’t know what it really does.

What am I talking about?

    try 
    {
        FutileAttemptToResist();
    }
    catch (BorgException err)
    {
         _myDearLog.Error("I'm in da cube! Ohh no!", err);
        throw err;
    }

Looks about right? The problem is the throw err; part. It will wipe the stack trace and create a new one from your catch block. You’ll never know where the exception originated from. Just use throw;

Using policies for exception handling

I’ve started to write several pieces about why you should not use policies (for instance Exception Handling Block in Enterprise library) but I never got really satisfied with it.

The problem with the block is that the examples is still not very good and really tricks you into bad practices.

Anyway, introducing policies can be good if you are following my guidelines in the post “Do NOT catch that exception!”. The problem though is that it’s easy to start adding try/catch/policy everywhere and hence producing unreadable code that is logging the exception multiple times again.

Just handle those exceptions that you can handle. No need for a policy.

Not including original exception when wrapping

Another common mistake is to not include the original exception when throwing another one. By failing to do so, you are also failing to give information about where the error really happened.

    try
    {
        GreaseTinMan();
    }
    catch (InvalidOperationException err)
    {
        throw new TooScaredLion("The Lion was not in the m00d", err); //<---- original exception is included, hooray!
    }

Not providing context information

This one is actually a golden rule at Microsoft. They really love to give us developers a challenge when something fails. They have built a superior framework with .Net and they need to put us developers back to earth sometimes. Hence the crappy exception descriptions.

This is what I’m talking about:

    try
    {
       socket.Connect("somethingawful.com", 80);
    }
    catch (SocketException err)
    {
        throw new InvalidOperationException("Socket failed", err);  //I LOVE InvalidOperationException. Read my previous post.
    }

What’s wrong? The exception message is just stating the obvious. We already know that the connect fails. But we do not know what it tried to connect to. Always provide contextual information. NHibernate is the glorious king when it comes to developer friendly exceptions. Examine it’s source code and learn something.

what you really should do is something like this:

    void IncreaseStatusForUser(int userId, int newStatus)
    {
        try
        {
             var user  = _repository.Get(userId);
             if (user == null)
                 throw new UpdateException(string.Format("Failed to find user #{0} when trying to increase status to {1}", userId, newStatus));
       
             user.Status = newStatus;
             _repository.Save(user);
        }
       catch (DataSourceException err)
       {
           var errMsg = string.Format("Failed to find modify user #{0} when trying to increase status to {1}", userId, newStatus);
            throw new UpdateException(errMsg, err);
       }

I always try to include as much information as possible in my exceptions. One could discuss if that sensitive information should not be included in exceptions. But I say YES! It’s really up to code that EXPOSES the exception (for instance a logger or a message box) to hide sensitive information. For instance, only log exceptions in a log that the correct people have access to. Exceptions should be dealt with ASAP anyway.

I would never expose a exception to a end user. What use do they have of the exception information? You got a widely used application and need to get exception information? Encrypt it and upload it to your server (with permission from your user of course).


Simplified localization for DataAnnotations

Update

I’ve come up with a better localization method for MVC3. Read about it here.

Original article

The built in localization support in DataAnnotations is a bit hard to work with. You are supposed to do add the ResourceManager to each attribute that you add. The code is looking like something like this:

    public class User
    {
        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        public int Id { get; set; }

        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        [StringLength(40, ErrorMessageResourceName = "Validation_StringLength", ErrorMessageResourceType = typeof(ModelTranslations))]
        public string FirstName { get; set; }

        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        [StringLength(40, ErrorMessageResourceName = "Validation_StringLength", ErrorMessageResourceType = typeof(ModelTranslations))]
        public string LastName { get; set; }
    }

You should of course move the strings to a static class or something like that.

The problem is that you have just added translation for the validation messages and not the actual model. There are actually no built in solutions for that problem.. If you google, you find some solutions using a custom LocalizedDisplayNameAttribute that uses the same approach as the DataAnnotationAttribute’s: Pointing on a resource type in the attribute constructor.

Updated source code for this solution:

    public class User
    {
        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        [LocalizedDisplayNameAttribute("User_Id", NameResourceType=typeof(ModelTranslations))]
        public int Id { get; set; }

        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        [StringLength(40, ErrorMessageResourceName = "Validation_StringLength", ErrorMessageResourceType = typeof(ModelTranslations))]
        [LocalizedDisplayNameAttribute("User_FirstName", NameResourceType=typeof(ModelTranslations))]
        public string FirstName { get; set; }

        [Required(ErrorMessageResourceName = "Validation_Required", ErrorMessageResourceType = typeof(ModelTranslations))]
        [StringLength(40, ErrorMessageResourceName = "Validation_StringLength", ErrorMessageResourceType = typeof(ModelTranslations))]
        [LocalizedDisplayNameAttribute("User_LastName", NameResourceType=typeof(ModelTranslations))]
        public string LastName { get; set; }
    }

Not very readable, is it? As you can see, I use the pattern ClassName_PropertyName for all strings definitions. It’s a safe way to avoid a name collision. The same word might not mean the same thing in different models.

The solution

What we need is a way to provide strings to all attributes WITHOUT specifying the resource in the attribute constructors. In this way we can easily move resources without having to change each attribute. Changing every attribute is not very DRY is it?

My solution is to create a singleton class that you can fetch strings from. Want a more flexible solution? Check my ShureValidation library (shameless advertising) that I’ve posted about earlier in this blog.

End result for the model:

    public class User
    {
        [Required]
        [LocalizedDisplayNameAttribute("User_Id")]
        public int Id { get; set; }

        [Required]
        [StringLength(40)]
        [LocalizedDisplayNameAttribute("User_FirstName")]
        public string FirstName { get; set; }

        [Required]
        [StringLength(40)]
        [LocalizedDisplayNameAttribute("User_LastName")]
        public string LastName { get; set; }
    }

A bit more readable? oooooh yes *uhu*.

Classes making it possible:

You have to derive all standard DataAnnotation attribute classes like this:

    public class RequiredAttribute : System.ComponentModel.DataAnnotations.RequiredAttribute
    {
        private string _displayName;

        public RequiredAttribute() 
        {
            ErrorMessageResourceName = "Validation_Required";
        }

        protected override ValidationResult IsValid(object value, ValidationContext validationContext)
        {
            _displayName = validationContext.DisplayName;
            return base.IsValid(value, validationContext);
        }

        public override string FormatErrorMessage(string name)
        {
            var msg = LanguageService.Instance.Translate(ErrorMessageResourceName);
            return string.Format(msg, _displayName);
        }
    }

Next step is to add a custom DisplayName class:

    public class LocalizedDisplayNameAttribute : DisplayNameAttribute
    {
        private PropertyInfo _nameProperty;
        private Type _resourceType;

        public LocalizedDisplayNameAttribute(string className, string propertyName)
            : base(className + (propertyName == null ? "_Class" : ("_" + propertyName)))
        {

        }

        public override string DisplayName
        {
            get
            {
                return LanguageService.Instance.Translate(base.DisplayName) ?? "**" + base.DisplayName + "**";
            }
        }
    }

And finally create the language service:

    public class LanguageService
    {
        private static LanguageService _instance = new LanguageService();
        private List<ResourceManager> _resourceManagers = new List<ResourceManager>();

        private LanguageService()
        {
        }

        public static LanguageService Instance { get { return _instance; } }

        public void Add(ResourceManager mgr)
        {
            _resourceManagers.Add(mgr);
        }

        public string Translate(string key)
        {
            foreach (var item in _resourceManagers)
            {
                var value = item.GetString(key);
                if (value != null)
                    return value;
            }

            return null;
        }
    }

You can easily switch to a database or something like that if you need to.

Update

A follow up entry showing how to fix client-side validation in MVC3 can be found here.


InvalidOperationException: “Exception” in disguise? Or just the new ApplicationException?

Microsoft added ApplicationException with the idea that all exceptions that you made should derive it. Fair point, your should be able to handle your own exceptions, right? The problem is what nobody used it and therefore Microsoft changed it’s recommendation to NOT use it. It’s just lying there in the framework like an old aunt that just don’t want to go away (I still love you all my dear aunts).

InvalidOperationException is also an exception that should be blown to hell. The description of the exception says this:

The exception that is thrown when a method call is invalid for the object’s current state.

What does that mean in the real world? When do exceptions get thrown in general? Well. When something unexpected have happened, as in when a method is called when it isn’t supposed to be. How else can an exception be raised? When not calling a method?

The InvalidOperationException is really like an Exception in a disguise. Try to search in a couple of open source projects after it (or your own). It’s thrown a lot. It’s a poor mans choice to exception handling. Ohhh, I’ve been guilty one too many times too. It’s so handy since it can be applied to all exceptional cases.

The reason to why I wrote this post is that I was debugging an ASP.Net MVC application and found that MS throws it when an action cannot be found in a controller. The result is that a Internal Server Error page is shown instead of a File Not Found page. Sure, it’s an error in the application and the proper controller have been found. But do the browser care about which framework is used in the web server? No. File not found is therefore the proper status code.

Microsoft should have created an ActionNotFound exception instead and derived it from HttpException (and set the 404 status code in the constructor initialization).

Edit:
I’ve filed a bug report for ASP.Net MVC: http://aspnet.codeplex.com/workitem/7588


Why autogenerated mstests is bad for you!

ms can generate complete test classes for you that test and access all methods and properties, no matter if they are private or public. Isn’t that awesome? Well no.

Let me do a statement first: I’m not fond of auto-generated code, no matter what it is for.

Why? Because writing code makes you think. You’ll have a better chance to find design error earlier than if you get a lot of stuff generated for you. You also need to know how stuff works. If everything is auto-generated for you, you don’t have to care. Right?

Back to mstests.

Reason 1:
The access to private/protected members/properties is bad for you. If you can’t manage to test those methods/properties through the public API it’s most likely that they can’t be called from your regular application either. Either remove the code that you can’t test or refactor the class.

Reason 2:
The  generated test classes includes one test per method. Inexperienced unit testers might think that it’s enough with one test per method: They either just write one test or includes all the different tests in the same test method. Both of those options make it a lot harder to test and get robust applications.

Reason 3:
Readability.The generated tests is harder to read:

(i’ll post samples here later)

Conclusion: Build all your tests by yourself (it makes you think about your design once more). Remove code that you can’t test through the public interface (or refactor).


Do NOT catch that exception!

Ohhh, I’ve recently seen one to many application where the developer try to be safe by catching exceptions all over the place. It can be everything from a small simple method that doesn’t throw that many exceptions to a large method with multiple try/catch statements. Most of those catch blocks just logs the exception and rethrow it.

Please do not catch that exception. Let it travel down the call stack. I promise you that you won’t regret it. You’ll get a warm and fuzzy feeling next time you’ll test your application. I promise you! Really!

Why? You’ll get a complete exception with all the details you need to fix the error, it’s very easy to loose information if you catch/wrap/rethrow exceptions . The problem with all of those try/catch blocks is they doesn’t help your application in any way. They just clutter up the code and your log files.

If you still want to catch exceptions: I’ll try to explain the way to do it.

Catching exceptions in the correct way.

Yeah. Proper exception handling can be something wonderful. You’ll smile every time you get an exception. First of all: Exceptions only happens when something exceptional occurs.  Please have that in mind, because it helps you remember that you don’t need those try/catch block everywhere.

That brings us to the golden rule about when to catch exceptions:

Only catch exceptions that you can handle to rescue the situation.

That means that you should only catch exceptions if you, by handling it, can let the application continue as (almost) expected. Let me rephrase that: Only catch a exception if you, by catching it, can let the method return a result as promised.

There are of course exceptions, namely two of them:

Do not let layer specific exceptions propagate up the call stack.

For instance, if the data layer fails and throws a SqlException (if you are using Sql Server) you should catch it and wrap it inside a more generic exception. Create a DataSourceException which could be used even if you switch data source from a database to a Web Service. You should of course add details to the exception such as the method name, parameters or anything else that can be useful. Do NOT forget to add the original exception as  inner exception.

public class UserRepository : IUserRepository
{
    public IList<User> Search(string value)
    {
        try
        {
              return CreateConnectionAndACommandAndReturnAList("WHERE value=@value", Parameter.New("value", value));
        }
        catch (SqlException err)
        {
             var msg = String.Format("Ohh no!  Failed to search after users with '{0}' as search string", value);
             throw new DataSourceException(msg, err);
        }
    }
}

Update 2013-03-01: I’m not following this rule any more. The only time I wrap exceptions is if I add more context information to help me prevent the exception in the future. It doesn’t really matter if the data layer exception is passed up the call stack, since it won’t be handled but just logged in the top layer

If you don’t want to display data layer specific information to the user, then simply display something like "An unexpected error occurred" instead of showing exception information. The exception information doesn’t really help your user either way.

Try/Catch all is OK in the layer closest to the user.

Ok, so you have let the exceptions propagate through all your layers to the presentation layer (winforms/asp.net etc). Fine. Go ahead, give the user some fancy error pages. Also log those errors. But just don’t log them. Notify a support team. Send an email to the project lead.

When everything else fails

Are you not sure that your new exception policy will catch all of those exceptions? Is it scary? The I got the solution for YOU!

Asp.Net Implement Aplication_OnError(object source, EventArgs e) in your global.asa. It will be called for all unhandled exceptions.
WinForms Subscribe on Application.ThreadException
WinServices Subscribe on AppDomain.CurrentDomain.UnhandledException. The problem with this event is that it will still terminate the application. But there’s a way around that too.
WCF WCF: Implement IErrorHandler and log the exceptions in it (added 2013-03-01).
ASMX ASMX: Create a custom Soap Extension. (added 2013-03-01).

In the next blog post I’ll discuss common mistakes in those catch blocks and how your tailor made exceptions should look like.

Update 2013-04-17

I’ve written a new exception series. Starting with What are exceptions?