CODEWITHSUNDEEP
c#exception
Marko
Marko

Reputation: 72222

Is this good practice for a Custom Exception?

public class PageNotFoundException : HttpException
{
    public PageNotFoundException()
        : base(404, "HTTP/1.1 404 Not Found")
    {

    }
}

The idea is that rather than typing this each time

throw new HttpException(404, "HTTP/1.1 404 Not Found")

I'd rather write

throw new PageNotFoundException();

I was going to add an overload for including the innerException however I will never use this in a try/catch block.

Would you consider this good practice?
i.e. Inheriting from an exception and passing hardcoded information to base(...).

Upvotes: 8

Views: 2335

Answers (3)

zmbq
zmbq

Reputation: 39013

If you are the one throwing the exception in the first place, then yes - it's OK. However, if you catch an HttpException and then try to throw a PageNotFoundException instead, you should put the original exception as the InnerException.

Upvotes: 2

David Anderson
David Anderson

Reputation: 13670

I decided to rewrite my answer to be specific to your actual question, and in a more broad sense that an MVC application isn't the only thing these best-practices apply to.

(1) Answer. This is not good practice. You should use a exception builder method instead that throws HttpException directly.

public static void ThrowPageNotFoundException() {
    throw new HttpException((Int32)HttpStatusCode.NotFound, "HTTP/1.1 404 Not Found");
}

(2) DO. Use exception builder methods (eg. the code I provided). This allows for you to avoid the extra performance cost of having your own exception type, and allows for it to be inlined. Members throwing exceptions do not get inlined. This would be the proper substitute for convenience throwing.

(3) DO. Use base class library exceptions whenever possible, and only create a custom exception when there is absolutely no base exception that meets the needed requirements. Creating custom exceptions adds deeper exception hierarchy, which makes debugging harder when it does not need to be, adds extra performance overhead, and also adds extra bloat to your code base.

(4) Do NOT. Throw the base class System.Exception. Use a specific exception type instead.

(5) Do NOT. Create custom exceptions for convenience. This is not a good reason for a custom exception, because exceptions are intrinsically costly.

(6) Do NOT. Create custom exceptions just to have your own exception type.

(7) Do NOT. Throw exceptions that can be avoided by changing the calling code. This would suggest that you have a usability error in the API rather than an actual problem.

Anyone who has read Framework Design Guidelines from the .NET development series will know these practices, and they are very good practices. These are the very practices that the .NET framework was built upon, and MVC as well.

Upvotes: 8

Tanzelax
Tanzelax

Reputation: 5714

While this is a nice construct in your own code for your own use, one consideration is that it can promote coding by convention which can be dangerous when you're dealing with other/new developers.

In your own libraries, if you are consistent about throwing a PageNotFoundException whenever a 404 HttpException should be thrown, it might make more sense to catch (PageNotFoundException). However, when you start using other libraries that don't have your custom exception, you will miss 404 HttpExceptions thrown by other code. Likewise, if you have other developers contributing at a later date (or even your own additions in the future), the consideration that PageNotFoundExceptions are what's being caught by most of the functionality may be missed and new 404 HttpExceptions could be thrown in the new modules, which would likewise not be caught by copy/pasted calling code.

Basically, constructs like this increase the acclimation time required for working on the project, and should be handled in such a way that this cost is minimized (made sufficiently visible in an easy to find central shared objects library that isn't already too cluttered).

On the other hand, there is certainly value in centralizing the generation of your HttpExceptions if you're what looking for is essentially the factory pattern benefits; it may be worth just going with that instead if that's what you're trying to get out of it (throw ExceptionFactory.NewPageNotFound()).

Upvotes: 1

Related Questions