Click here to Skip to main content
15,888,521 members
Articles / Programming Languages / C#

C# BAD PRACTICES: Learn how to make a good code by bad example – Part 4

Rate me:
Please Sign up or sign in to vote.
4.88/5 (73 votes)
23 Mar 2017CPOL22 min read 91.9K   110   29
The most popular mistakes of Exception Handling and Logging

Introduction

Hi all,

I would like to present you my point of view on one of the most important part of the Software Developer's work – Exception Handling and Logging.

While many years of working on a different types of applications in a different companies I was changing opinion on this topic many times. But after that experience I have a final point of view on how Exception Handling and Logging should work IN MY OPINION. I want to share with you how do I see it – of course using bad examples – the same as in three previous articles in this series!

You can find my three previous articles in this series here:

C# BAD PRACTICES: Learn how to make a good code by bad example
C# BAD PRACTICES: Learn how to make a good code by bad example – Part 2
C# BAD PRACTICES: Learn how to make a good code by bad example – Part 3
 

To understand this article, you don't have to read any of my previous articles, but I encourage to do so :)

There is no “one source of true”

How should we handle exception always depends on situation. What kind of application it is? What are the requirements? What kind of exception it is?

However there are some common rules which in my opinion should be followed in every properly written enterprise application.

Why is it important?

When the application is working in the production environment, it is extremely important from the maintenance perspective to have a good exception handling and even more important – to log those exceptions. Bad written code could introduce for example “a silent killers” which can cost the company much money.

Let's imagine, that somebody is trying to buy some product in an online shop. One step of the process fails for a specific data. Error is not logged. A customer resign from shopping in this online shop and does not report a problem. He is so angry and stops relying on our service. He switches to a competitor. And this situation is happening for one year, for a particular scenario. Can you imagine how much money the company will lose?

This is only one example of many situations, which can happen if you don’t follow good practices while implementing exception handling and logging :)

The form of the article

In the article I will present 4, the most common mistakes I came across while working as a software developer. I will show a real world example of a C# code which makes troubles and then present how in my opinion this code should be written properly. I’ve added a business context to each example to show that this situation can impact a business of our employer.

As I'm ALWAYS highlighting, these code snippets are only examples, NOT FULLY WORKING APPLICATION and I'm only using them to show an approach. I also assume, that sometimes you will have to use some legacy components, which were not written by yourself and can contain “not ideal code”, on which you can’t fully rely.

Instead of writing “you should always follow this, you should never do that”, I will try to show what consequences can these mistakes bring us in practice.

To understand the article, you will only need a basic knowledge of the C# language.

1. Eating exceptions

This is my favourite one.

Imagine online shop in which, customers have to register to our system and then, they can buy products. Everybody can imagine how annoying is the process of registering but anyway...

We have the UserService class which is handling our customer's registration process. It creates a user in the database, generates an activation link, stores it in the database and then sends a message to our queueing system. Another component – e-mail sender – subscribes to this queue. It's getting a messages from it and sending an e-mail with the activation link to our customers. A common functionality.

Now the developer who was implementing the RegisterUser method in the UserService class got a requirement, that if all steps of register process will execute without exceptions but there will be an error while sending a message to the queue, then user should not be notified about the error as this is an internal problem. User should not be bothered about some network or configuration problem as all important data was stored in the database and user definition exists in it.


So our developer implemented below class:

C#
public class UserService
{
    private readonly IMessagingService _messagingService;
    public UserService(IMessagingService messagingService)
    {
        _messagingService = messagingService;
    }

    public bool RegisterUser(string login, string emailAddress, string password)
    {
        // insert user to the database

        // generate an activation code to send via email

        // save activation code to the database

        // create an email message object
        try
        {
            _messagingService.SendRegistrationEmailMessage(message);
        }
        catch (Exception)
        {
            // boom!
        }
        return true;

    }
}

 

The code is simple, I just added the most important part for this example. It doesn't matter which queueing system we are using as in the example you only see a wrapper to a real implementation of writing to a queue, hidden by the interface.

As our developer is a funny guy, he wrote a funny comment in a catch block :)
Yes, I saw this comment in a serious business application indeed :)

As we have a Global Exception Handling (it will be explained in the Global Error Handling paragraph) implemented – there is a code which is handling all unhandled exceptions, logging them and returning an error page to the end user – all exceptions, except those in the try block, will be handled properly and user will see an error page.

If an exception will occur in a messaging service class (an implementation of the IMessagingService interface), user will still get an information that everything went fine.

It looks like our developer satisfied the requirements.

But...

He was very hungry and he ate the exception rather than logging it.

What consequences will it have?

 

Imagine now two situations:

A) after one of the deployments

  • deployment engineer messed up the configuration file and he made a typo in the address of the queue

  • queue was moved to another server and somebody forgot that our application is writing to it and didn’t change the address in the web.config file

B) there are very often network issues between our web application and the external server on which there is a queue installed or the server is down for a few hours from time to time, for some reasons

 

So...

Users are trying to register, system says that everything went fine. But they never got an activation e-mial, so they can't log in into the system. They're trying to register again, but system says that the user with this login/e-mai address already exists in the system. They're trying to register again with the new credentials but the result is the same. Do you think that they will contact a support or will choose a competitor?
Answer this question yourself.

 

I don't have to add that our logs are perfectly empty :)

 

Now, in case of the option A), problem will be probably noticed after some time as the owner will see that the number of new users doesn't increase and will start digging in.

But in case of the option B), we are facing a thing which I call “a silent killer”. Imagine that this situation is happening for years – how many customers will the company lose?


If our developer would have predicted this situation he would implement the code as follows instead:

C#
public class UserService
{
    private readonly IMessagingService _messagingService;
    private readonly ILogger _logger;
    public UserService(IMessagingService messagingService, ILogger logger)
    {
        _messagingService = messagingService;
        _logger = logger;
    }

    public bool RegisterUser(string login, string emailAddress, string password)
    {
        // insert user to the database

        // generate an activation code to send via email

        // save activation code to the database

        // create an email message object
        try
        {
            _messagingService.SendRegistrationEmailMessage(message);
        }
        catch (Exception ex)
        {
            _logger.Error(string.Format("Exception occurred while sending an activation emailto a user with login: {0}, email: {1}", login, emailAddress), ex.ToString());
        }
        return true;

    }
}

You can notice that we introduced a logger class, which is now logging a detailed exception.

We pass to the Error method a custom message and the original exception call stack and both will be written to the log file.

 

What does it change?


Now, when we will look into the log file we will notice that we have a serious problem in our registration process, because since the last deployment, an activation e-mails are not sent. The company will be even able to try to fix this situation by contacting a concrete people manually as we have their e-mails in the database. There also can be some “re-send an activation email to users” process implemented, but this is a detail.

2. Incorrect logging

Let’s change the context now. Imagine that our company has a complex website for its customers, written in the ASP.NET MVC. The only way to report the bug is via contact form. Let’s check how this feature (sending a message to the application support) is implemented…

MVC controller’s action implementation:

C#
[HttpPost]
public ActionResult SendMessage(MessageModel messageModel)
 {
   try
   {
     var emailAddress = _contactControllerHelper.GetEmailReceiverAddress(messageModel.Category); // line: 37
     // code responsible for sending an e-mail
     
   }
   catch(Exception ex)
   {
     _logger.Error(ex.Message);
     return Json(false);
   }
   return Json(true);
 }

we can notice, that the action takes a model with a message as a parameter, sent from the view using ajax call. Then depending on sent category (chosen from the drop down list on the contact form (UI)), _contactControllerHelper gets an e-mail address of the particular department.

Contact categories definition is stored in C# enum:

public enum ContactCategory
{
  NewCustomersSupport = 1,
  ApplicationSupport = 2,
  ExistingCustomersSupport = 3
}

 

And the implementation of _contactControllerHelper looks as below:

C#
public class ContactControllerHelper : IContactControllerHelper
{
  private readonly Dictionary<ContactCategory, string> _emailAddressesForContactCategories;
  public ContactControllerHelper(Dictionary<ContactCategory, string> emailAddressesForContactCategories)
  {
    _emailAddressesForContactCategories = emailAddressesForContactCategories;
  }
  public string GetEmailReceiverAddress(ContactCategory contactCategory)
  {
    return _emailAddressesForContactCategories[contactCategory]; // line: 22
  }
}

 

It keeps, passed via constructor, dictionary of assignments of e-mail addresses to categories, and returns an e-mail address of the department assigned category using GetEmailReceiverAddress method. If the dictionary doesn’t contain a definition of passed to  GetEmailReceiverAddress method category it will fail fast, and try-catch block in MVC controller’s action will catch the exception, log it and return a failure to the view, which will present a custom error page to the end-user.


We could deliberate what will be better:

  • try-catch in the action method

  • global error handling

  • logging implemented in an Aspect Oriented way

  • etc.


The problem I want to show here is unrelated to a method of exception handling.


So imagine now that the code which is passing a dictionary to the ContactControllerHelper instance, has a bug. For some reason it doesn’t contain an assignment definition for  the ApplicationSupport category, and it looks like that:

C#
var emailAddressesForContactCategories = new Dictionary<ContactCategory, string>() {      
     { ContactCategory.NewCustomersSupport, ConfigurationManager.AppSettings["NewCustomersSupportEmailAddress"] },
     { ContactCategory.ExistingCustomersSupport, ConfigurationManager.AppSettings["ExistingCustomersSupportEmailAddress"] }
     };

 

So when the end-user will try to report a bug (send a message to an Application Support), the system will display to him an error page and we will see in a log file, below entry:

"The given key was not present in the dictionary."
 

As our application is quite big, from above information we can say completely nothing.

We can’t even say which part of our system has a problem.


So to sum up, we see that we have a problem in our application but we can’t localize it. No bug has been reported from our end-users as they basically can’t report it :)

I don’t think that our users will be happy of the product we deliver to them.
I would start looking for the competitors...
 

So why do we have so little information about the exception in our log file?

Let’s take a look at the catch block in our action method:

C#
catch(Exception ex)
{
  _logger.Error(ex.Message);
  return Json(false);
}

And this is our real problem:

C#
_logger.Error(ex.Message);

Our developer made a mistake. Instead of logging full stack of the exception, he logged only a general message, using ex.Message property. How can we fix it? Very easily. Let’s replace our old catch block with a new one:

C#
catch(Exception ex)
{
  _logger.Error(ex.ToString());
  return Json(false);
}

 

Now check what will we see in a log file:

"System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.

at System.Collections.Generic.Dictionary`2.get_Item(TKey key)

at ExceptionTest.Helpers.ContactControllerHelper.GetEmailReceiverAddress(ContactCategory contactCategory)

in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\Helpers\\ContactControllerHelper.cs:line 22

at ExceptionTest.Controllers.ContactController.SendMessage(MessageModel messageModel)

in c:\\[PATH_TO_PROJECT]\\ExceptionTes\\Controllers\\ContactController.cs:line 37"

 

You can see now, that we have a full call stack of the exception stored in our log file.

We can easily say now that we have a problem with sending message from the contact form and that the key for one of the Contact Categories doesn’t exist in the _emailAddressesForContactCategories dictionary, because reported in a log file:


ContactControllerHelper.cs:line 22
is:

C#
return _emailAddressesForContactCategories[contactCategory];

One line changed very much.




Obviously, the best solution would be to have:

C#
public string GetEmailReceiverAddress(ContactCategory contactCategory)
{
   string emailAddress;

   if (!_emailAddressesForContactCategories.TryGetValue(contactCategory, out emailAddress))
   {
     throw new ArgumentOutOfRangeException(string.Format("There is no e-mail address definition for the given category id: ({0})", contactCategory));
   }

   return emailAddress;
}

instead of:

C#
public string GetEmailReceiverAddress(ContactCategory contactCategory)
{
  return _emailAddressesForContactCategories[contactCategory];
}

 

Which would tell us everything what we need, but very often (especially in a huge projects with a lot of legacy code), we have to deal with “not perfect code”, like consuming an old, shared class and as this class is used in many modules, we won’t want to change it’s behaviour (to avoid introducing a bug).


That’s why in any case, we need to have properly implemented exception handling and logging.

3. Re-throwing exceptions and method context logging

 

Another example.

 

Let’s imagine that we have a system (ASP.NET MVC website), where on UI(Admin Panel), we can search for the user by the User Name. The requirement is that if the searched user doesn’t exist or is not active, our end-user will see the screen with:

  • "User doesn't exist" text -  as a form header, instead of the User Name

  • “---“  - as a FirstName, Surname and GroupName

 

If the user was found, system will display correct values from the database.

 

If an exception has occurred, we want our exception to bubble up, because we have a Global Error Handling (it will be explained in the Global Error Handling paragraph) for the application in place, so this exception will be caught, logged, and user friendly message will be presented to the user (for example, Custom Error Page).

 

To implement that, our developer chose Null Object Design Pattern to avoid dealing with nulls and if-else logic.

The NullUserViewModel class will look like:

C#
public class NullUserViewModel : IUserViewModel
{
  public NullUserViewModel()
  {
    UserName = "User doesn't exist";
    FirstName = "---";
    Surname = "---";
    GroupName = "---";
  }

  public string UserName { get; set; }
  public string FirstName { get; set; }
  public string Surname { get; set; }
  public string GroupName { get; set; }
}

 

Our developer hit an idea to log some more descriptive information to the log file if exception will occur. He wanted people who don't have an access to a source code at all or don't have this access at the time of looking into the logs, to have some general idea which functionality is not working correctly.

 

Fair enough, in my opinion every information which improves visibility of system’s problems is an advantage.

 

Let's check how did he implement it:

C#
public class UserService
{
   private readonly ILogger _logger;
   private readonly IRepository _repository;

   public UserService(ILogger logger, IRepository repository)
   {
       _logger = logger;
       _repository = repository;
   }

   public IUserViewModel GetUserInfo(string userName)
   {
       try
       {
           var user = _repository.GetUser(userName);
           if (user != null && user.IsActive)
           {
             return new UserViewModel() // line: 27
             {
               FirstName = user.FirstName,
               Surname = user.Surname,
               GroupName = user.Group.Name
             };
           }
           return new NullUserViewModel();
       }
       catch (Exception ex)
       {
           _logger.Error("An error occurred while trying to get user data from the database.");
           throw ex; //line: 39
       }
   }
}

You can notice that the method GetUserInfo is returning NullUserViewModel class only, when an active user was not found. The code in the catch block writes detailed information to a log file and re-throws an exception.

 

The idea of the developer was that when somebody will open the log file, he will be able to say which part of the system, from the business perspective, is not working. Digging into the code takes more time and needs a developer knowledge and the source code. So for example, if the registration or logging module is not working, we have an alarm and we need to fix it immediately, but if the system does not return one of FAQ sections, it's not as critical.

 

So now, let's put yourself into the shoes of the developer who is maintaining this code.

Imagine, that an exception occurs in the try block of our GetUserInfo method. And it is: NullReferenceException.

 

User sees a pretty error message, our application logs the following:

"An error occurred while trying to get user data from the database.
System.NullReferenceException: Object reference not set to an instance of an object.at ExceptionTest.UserService.GetUserInfo(String userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 39

at ExceptionTest.Controllers.AdminController.GetUser(string userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\Controllers\\AdminController.cs:line 40
"


 

As you can notice, we don't see in which line the original exception occurred. That's because re-throwing exception was implemented incorrectly:

C#
throw ex;

 

The above line, erases the call stack of the original exception. You can notice that the bottom of the stack is showing UserService.cs:line 39, which is:

C#
throw ex;

 

So even if we will log an error properly in the global error handler (using ex.ToString()), we won’t see the original call stack.

 

Another thing is that our additional information doesn't help us at all, because after we checked the reported line, we already know that.

 

What can we say now? Can we say what exactly had happened?

 

So there are few possible scenarios:

  • The exception occurred while executing:
    user.Group.Name
    The Group property of the User class was not populated and it is null.

  • The exception occurred while executing below code:
    _repository.GetUser(userName);
    null was passed into the constructor of our UserService as a repository object and while trying to access this service an exception takes place.
    If exception occurs only from time to time for some special conditions, then this case is rather unlikely.
    But maybe there is another consumer of the UserService class which is placed in another rarely used module – then this option is possible.

  • The exception occurred somewhere in a pipeline of methods which starts from:
    _repository.GetUser(userName);
    There may be X methods in the pipeline and a lot of places where it could take place. Let’s say that one of them is on the Data Access Layer level.


 

Moreover, even if we can do a migration of the production data to the development database, we won't be able to reproduce the exact case in the development environment, because we don't know what userName was requested when the exception occurred.

 

So too many unknowns, too many actions to do. Remember, that this is an extremely simple example. Imagine how difficult would it be if we would have more components consumed in the try block and more places where the potential exception might occur.

 

Probably a long investigation and a trial and error method would finally give you the answer, but as we don't want to treat our application like a black box

let's modify a little bit our catch block:

C#
catch (Exception ex)
{
    _logger.Error(string.Format("An error occurred while trying to get user data from the database. UserName = {0}",userName));
    throw; // line: 39
}


 

we've changed:

C#
throw ex;

to

C#
throw;

 

Now we are following the well known rule “Always use throw; while re-throwing exception to preserve the original exception call stack”.

 

And we've also enriched our additional logging information to add a context of the method call:

C#
string.Format("An error occurred while trying to get user data. UserName = {0}", userName)

Logging input parameter (userName) will allow us to reproduce the same situation if it will be necessary.
 

And now check what do we see in a log file:

"An error occurred while trying to get user data from the database. UserName = Test.
System.NullReferenceException: Object reference not set to an instance of an object.

at ExceptionTest.UserService.GetUserInfo(String userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 39
at ExceptionTest.Controllers.AdminController.GetUser(string userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\Controllers\\AdminController.cs:line 40
"

 

Surprised?

We see now additional information:

“UserName = Test”, which allows us to reproduce the issue if we have an original data, but the exception call stack is still the same as before changing:

C#
throw ex;

to

C#
throw;

 

Why?
We’ve hit the special case.

 

IMPORTANT: 
If we are re-throwing an exception which has occurred in the method where this exception was catched, then the thrown exception will not preserve the original call stack (even if we're using throw;).

If method A would catch an exception which occurred in method B (called from method A), and we re-throw it using:

C#
throw;

then, thrown exception would contain the original exception call stack.

 

So how can we preserve the original exception call stack if the exception occurred in the same method?

 

We can preserve the original call stack using below code:

C#
MethodInfo preserveStackTrace = typeof(Exception).GetMethod("InternalPreserveStackTrace",

             BindingFlags.Instance | BindingFlags.NonPublic);

preserveStackTrace.Invoke(exception, null);


I’ve found this trick here:

https://weblogs.asp.net/fmarguerie/rethrowing-exceptions-and-preserving-the-full-call-stack-trace

 

Above code can be implemented as an Extension Method of the exception type:

public static class ExceptionExtensions
{
   public static void PreserveStackTrace(this Exception exception)
   {
       MethodInfo preserveStackTrace = typeof(Exception).GetMethod("InternalPreserveStackTrace",
       BindingFlags.Instance | BindingFlags.NonPublic);
       preserveStackTrace.Invoke(exception, null);
       }
   }
}

And then our new, catch block will look like this:

C#
catch (Exception ex)
{
    ex.PreserveStackTrace();
    _logger.Error(string.Format("An error occurred while trying to get user data from the database. UserName = {0}", userName));
    throw; // line: 40
}

 

Let’s check what will we see in a log file now:

"An error occurred while trying to get user data from the database. UserName = Test.
System.NullReferenceException: Object reference not set to an instance of an object.

at ExceptionTest.UserService.GetUserInfo(String userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 27
at ExceptionTest.UserService.GetUserInfo(String userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 40
at ExceptionTest.Controllers.AdminController.GetUser(string userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\Controllers\\AdminController.cs:line 40
"

Finally!

We got an information where the real exception has occurred, it's line 27, which is:

return new UserViewModel()
{
    FirstName = user.FirstName,
    Surname = user.Surname,
    GroupName = user.Group.Name
};


 

I can personally see some drawbacks of the above solution, we’re basing on the reflection, not on the given by the .Net Framework api, it’s some kind of hack.

In theory, in the next versions of the framework, there can be some internal change done and this solution might break.

 

So how can we better our code to get the original exception in our log file and at the same time use the given by the Microsoft api?

 

Let’s change our catch block to:

catch(Exception ex)
{
    throw new Exception(string.Format("An error occurred while trying to get user data from the database. UserName = {0}", userName), ex); // line: 38
}


We can basically enrich an exception, which anyway will be logged by our Global Error Handling (it will be explained in the Global Error Handling section). The new, thrown exception will contain an original exception stack trace in the InnerException field.

 

Thank to that we will be able to remove logger from the UserService class:

C#
public class UserService
{
  private readonly IRepository _repository;

  public UserService(IRepository repository)
  {
    _repository = repository;
  }

  public IUserViewModel GetUserInfo(string userName)
  {
    try
    {
      var user = _repository.GetUser(userName);
      if (user != null && user.IsActive)
      {
        return new UserViewModel() // line: 28
        {
          FirstName = user.FirstName,
          Surname = user.Surname,
          GroupName = user.Group.Name
        };
      }
      return new NullUserViewModel();
    }
    catch (Exception ex)
    {
      throw new Exception(string.Format("An error occurred while trying to get user data from the database. UserName = {0}", userName), ex); // line: 38
    }
  }
}
 

 

Let’s check what do we have in a log file now:

"System.Exception: An error occurred while trying to get user data from the database. UserName = Test. --->

System.NullReferenceException: Object reference not set to an instance of an object.   

at ExceptionTest.UserService.GetUserInfo(String userName)

in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 28

  --- End of inner exception stack trace ---

at ExceptionTest.UserService.GetUserInfo(String userName)
in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\UserService.cs:line 38

at ExceptionTest.Controllers.AdminController.GetUser(string userName)

in c:\\[PATH_TO_PROJECT]\\ExceptionTest\\Controllers\\AdminController.cs:line 40"

 

Our global exception handler is logging exceptions properly, using:

C#
ex.ToString()

so that, both exception stack trace (of the new exception) and inner exception stack trace (of the original exception) will be logged.

 

And we’re still getting the line, where the original exception has occurred (line 28).

This is the approach I would recommend or this kind of issue.

 

Now, we know that:

either the group was not taken from the database or some C# code didn't assign an information about the group (returned from the database) to the user object.

 

We can check now if this particular user does have a group assigned in the production database (or ask people responsible for the database to check it). If no, we've found the problem, otherwise we have to examine group assignment code in C#.

If we won't be able to find anything, we can migrate the data to the development database and reproduce the exact situation, because we have an information about searched userName in the log file.

 

4. An exception to control the program flow

The last example will be a very simple method in the Data Access Layer. Imagine that we are using Entity Framework to get the user data from the database. If the user was not found, method should return just null.


Our poor developer implemented the below code:

C#
public class DAL
{
  // the rest of the code

  public User GetActiveUser(string userName)
  {
    try
    {
      return _dbContext.Users.First(x => x.UserName == userName && x.IsActive);
    }
    catch(Exception)
    {
      return null;
    }
  }
}

What problem do we have here?

 

Obviously this is a very dangerous code. If any exception will occur our method will return null. So if for example, the database server is down, there is a network connection problem or there is a bug in the connection string, our method will return null, the same as in case of the “User not found”. This code is hiding a potential, serious problems.

 

Obviously, we can change the code to caching only concrete exception type and the rest of them will be bubbling up, which will a much better solution. The problem will be noticed quickly.

The code will look like below:

C#
public class DAL
{
  // the rest of the code

  public User GetActiveUser(string userName)
  {
    try
    {
      return _dbContext.Users.First(x => x.UserName == userName && x.IsActive);
    }
    catch(InvalidOperationException)
    {
      return null;
    }
  }
}

 

But the code is still not ideal.

 

Why?

  • we will notice very little performance decrease as throwing an exception unnecessarily will always slow the program a little bit
  • there is a lot of unnecessary code – compare with improved version
  • readability is not so great
  • poor experience while debugging – this one drives me crazy - while debugging the code, every time there will be a request for the user which doesn't exist or is not active, the debugger will stop. Imagine that there is a lot of places with the similar code to our example. You start looking for the person who did that to you :)

Obviously you can set in Visual Studio: “Don't break when exception occurs”, but then you won't be noticed about the real exceptions, which you're interested in.

 

So how to fix it?

Below code uses FirstOrDefault() method on the IQueryable collection, which will return the value if it exists. If not, it will return null without throwing an exception. The code is more simpler and more readable.


This is what we want:

public class DAL
{
  // the rest of the code

  public User GetActiveUser(string userName)
  {
    return _dbContext.Users.FirstOrDefault(x => x.UserName == userName && x.IsActive);
  }
}

So to sum up this paragraph, if you can avoid exception to happen, do it.

 

I can understand that FirstOrDefault() method was introduced in version 3.5 of the .Net Framework, but hey, it’s been 9 years since that time ;)

 

The same situation is with Parse() and TryParse() methods etc.

 

Global Exception Handling

Who of you guys saw an applications with try-catch in every method? Does it look readable when there is a one line of what really method is doing opposed to X lines of try-catch block, exception logging and returning a specific response?

The same code is repeated in all methods manually. If someone will forget to write it in one of these methods, an exception will become unhandled. Is it the best approach?
I would say NO.

I rather prefer to implement a global exception handling.

 

How should it look like?

We're not using try-catch blocks in our application, except some special cases, like:

“The requirement is that if an error occurs while sending an e-mail, application should only log this exception and not bubble it up”.

 

All unhandled exceptions will be caught by a one piece of code, which will log it and serve a general message to end-user as:

  • an error page - in case of web application
  • a message box - in case of windows forms application
  • a response to request – in case of web api
  • etc.

 

I will not present any implementation of it as:

  • depend on the application type (desktop application, web application, windows service, console application) you will have to use a different mechanism

  • there is no “one way” of doing it

  • you could write X separate articles on this topic

  • THIS IS NOT THE MAIN TOPIC OF THIS ARTICLE

Summary

To sum up, my article explains:

  • why you shouldn’t eat exceptions

  • the difference between: Exception.Message and Exception.ToString()

  • the difference between: throw ex;, throw; and throw new Exception(“Method call context”, ex);

  • why usage of exceptions to control the program flow is not the best idea

Conclusion

In the article, I presented you how serious consequences the common exception handling and logging mistakes can have. Sometimes, the one line of code can make much difference and lead to many troubles.

The advice I can give you is:

“while implementing an exception handling and logging, put yourself in a developer - who will be maintaining this code - shoes and think how helpful will it be for him when the exception will occur”.

Thanks!

If you have any questions related to the article, don't hesitate to contact me!

Sources

Source of images used in the article:

https://openclipart.org/

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)


Written By
Ireland Ireland
Passionate, Microsoft Certified Software Developer(Senior),
having Masters Degree in Information Technology.

Comments and Discussions

 
QuestionGood for you!!! Pin
Assil23-Jan-18 2:19
professionalAssil23-Jan-18 2:19 
If only you know how many people make these mistakes and they consider themselves experienced professionals.
Thank you for the examples, I will forward that to my developers.

AnswerRe: Good for you!!! Pin
Radosław Sadowski24-Jan-18 4:58
Radosław Sadowski24-Jan-18 4:58 
GeneralMy vote of 5 Pin
Bryian Tan7-Apr-17 14:53
professionalBryian Tan7-Apr-17 14:53 
GeneralRe: My vote of 5 Pin
Radosław Sadowski7-Apr-17 23:05
Radosław Sadowski7-Apr-17 23:05 
QuestionExcellent Pin
Julio Robles29-Mar-17 19:32
professionalJulio Robles29-Mar-17 19:32 
AnswerRe: Excellent Pin
Radosław Sadowski29-Mar-17 20:54
Radosław Sadowski29-Mar-17 20:54 
QuestionSyntactic Jello Pin
YrthWyndAndFyre27-Mar-17 7:26
YrthWyndAndFyre27-Mar-17 7:26 
AnswerRe: Syntactic Jello Pin
Radosław Sadowski29-Mar-17 2:43
Radosław Sadowski29-Mar-17 2:43 
QuestionInteresting Pin
Yves25-Mar-17 7:50
Yves25-Mar-17 7:50 
GeneralAn exception is just that - an exception Pin
Emmerel24-Mar-17 6:43
Emmerel24-Mar-17 6:43 
GeneralMy vote of 5 Pin
chaz-chance24-Mar-17 4:00
chaz-chance24-Mar-17 4:00 
GeneralRe: My vote of 5 Pin
Radosław Sadowski24-Mar-17 5:56
Radosław Sadowski24-Mar-17 5:56 
QuestionALternative to the exception extention method Pin
williams200016-Mar-17 23:47
professionalwilliams200016-Mar-17 23:47 
AnswerRe: ALternative to the exception extention method Pin
Radosław Sadowski17-Mar-17 3:01
Radosław Sadowski17-Mar-17 3:01 
PraiseMy vote of 5 Pin
lwbritz16-Mar-17 4:04
lwbritz16-Mar-17 4:04 
GeneralRe: My vote of 5 Pin
Radosław Sadowski16-Mar-17 7:36
Radosław Sadowski16-Mar-17 7:36 
SuggestionBad/Lazy even Useless exception handling, not done properly at all. Pin
lopatir16-Mar-17 0:32
lopatir16-Mar-17 0:32 
GeneralRe: Bad/Lazy even Useless exception handling, not done properly at all. Pin
Radosław Sadowski16-Mar-17 2:01
Radosław Sadowski16-Mar-17 2:01 
GeneralRe: Bad/Lazy even Useless exception handling, not done properly at all. Pin
Ramon Ll. Felip21-Mar-17 5:47
Ramon Ll. Felip21-Mar-17 5:47 
GeneralMy vote of 4 Pin
dmjm-h15-Mar-17 11:35
dmjm-h15-Mar-17 11:35 
GeneralRe: My vote of 4 Pin
Radosław Sadowski15-Mar-17 22:02
Radosław Sadowski15-Mar-17 22:02 
QuestionI'm Passionate About This Too! Pin
diverbw6-Mar-17 8:02
professionaldiverbw6-Mar-17 8:02 
AnswerRe: I'm Passionate About This Too! Pin
Radosław Sadowski6-Mar-17 9:41
Radosław Sadowski6-Mar-17 9:41 
GeneralMy vote of 5 Pin
_Vitor Garcia_6-Mar-17 6:04
_Vitor Garcia_6-Mar-17 6:04 
GeneralRe: My vote of 5 Pin
Radosław Sadowski6-Mar-17 6:44
Radosław Sadowski6-Mar-17 6:44 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Praise Praise    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.