38

i was working with a small routine that is used to create a database connection:

Before

public DbConnection GetConnection(String connectionName)
{
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);
   DbConnection conn = factory.CreateConnection();
   conn.ConnectionString = cs.ConnectionString;
   conn.Open();

   return conn;
}

Then i began looking into the .NET framework documentation, to see what the documented behaviour of various things are, and seeing if i can handle them.

For example:

ConfigurationManager.ConnectionStrings...

The documentation says that calling ConnectionStrings throws a ConfigurationErrorException if it could not retrieve the collection. In this case there is nothing i can do to handle this exception, so i will let it go.


Next part is the actual indexing of the ConnectionStrings to find connectionName:

...ConnectionStrings[connectionName];

In this instance the ConnectionStrings documentation says that the property will return null if the connection name could not be found. i can check for this happening, and throw an exception to let someone high up that they gave an invalid connectionName:

ConnectionStringSettings cs= 
      ConfigurationManager.ConnectionStrings[connectionName];
if (cs == null)
   throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

i repeat the same excercise with:

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);

The GetFactory method has no documentation on what happens if a factory for the specified ProviderName couldn't be found. It's not documented to return null, but i can still be defensive, and check for null:

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);
if (factory == null) 
   throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

Next is construction of the DbConnection object:

DbConnection conn = factory.CreateConnection()

Again the documentation doesn't say what happens if it couldn't create a connection, but again i can check for a null return object:

DbConnection conn = factory.CreateConnection()
if (conn == null) 
   throw new Exception.Create("Connection factory did not return a connection object");

Next is setting a property of the Connection object:

conn.ConnectionString = cs.ConnectionString;

The docs don't say what happens if it could not set the connection string. Does it throw an exception? Does it ignore it? As with most exception, if there was an error while trying to set the ConnectionString of a connection, there's nothing i can do to recover from it. So i'll do nothing.


And finally, opening the database connection:

conn.Open();

The Open method of DbConnection is abstract, so it's up to whatever provider is descending from DbConnection to decide what exceptions they throw. There is also no guidance in the abstract Open methods documentation about what i can expect to happen if there's an error. If there was an error connecting, i know i cannot handle it - i'll have to let it bubble up where the caller can show some UI to the user, and let them try again.


After

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
    if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

Summary

So my four line function, became 12 lines, and required 5 mintues of documentation lookup. In the end i did catch one case where a method is allowed to return null. But in practice all i did was convert an access violation exception (if i attempts to call methods on a null reference) into an InvalidArgumentException.

i also catch two possible cases where there could be null return objects; but again i only traded one exception for another.

On the positive side, it did catch two problems, and explain what happened in the exception message, rather than bad things happening down the road (i.e. the buck stops here)

But is it worth it? Is this overkill? Is this defensive programming gone awry?

3
  • 2
    This is quite good. The only thing I'd do differently is to use String.Format to build the messages instead of appending strings. I'd also put the messages into resources, to permit localization. Jun 27, 2009 at 17:34
  • 3
    Which routine is easier to debug? Which is easier to maintain? Does the added complexity gain you enough to make it worthwhile?
    – Nosredna
    Jun 28, 2009 at 19:58
  • 3
    Well, the easiest code to write is one that has no error checking at all. And code without any kind of error checking is also easier to read.
    – Ian Boyd
    Jun 29, 2009 at 13:13

14 Answers 14

31

Manually checking for a configuration and throwing an exception is no better than just letting the framework throw the exception if the configuration is missing. You are just duplicating precondition checks which happens inside the framework methods anyway, and it makes you code verbose with no benefit. (Actually you might be removing information by throwing everything as the base Exception class. The exceptions thrown by the framework are typically more specific.)

Edit: This answer seem to be somewhat controversial, so a bit of elaboration: Defensive programming means "prepare for the unexpected" (or "be paranoid") and one of the ways to do that is to make lots of precondition checks. In many cases this is good practice, however as with all practices cost should be weighed against benefits.

For example it does not provide any benefit to throw a "Could not obtain connection from factory" exception, since it doesn't say anything about why the provider couldn't be obtained - and the very next line will throw an exception anyway if the provider is null. So the cost of the precondition check (in development time and code complexity) is not justified.

On the other hand the check to verify that the connection string configuration is present may be justified, since the exception can help tell the developer how to resolve the problem. The null-exception you will get in the next line anyway does not tell the name of the connection string that is missing, so your precondition check does provide some value. If your code is part of a component for example, the value is quite big, since the user of the component might not know which configurations the component requires.

A different interpretation of defensive programming is that you should not just detect error conditions, you should also try to recover from any error or exception that may occur. I don't believe this is a good idea in general.

Basically you should only handle exceptions that you can do something about. Exceptions that you cannot recover from anyway, should just be passed upwards to the top level handler. In a web application the top level handler probably just shows a generic error page. But there is not really much to do in most cases, if the database is off-line or some crucial configuration is missing.

Some cases where that kind of defensive programming makes sense, is if you accept user input, and that input can lead to errors. If for example the user provides an URL as input, and the application tries to fetch something from that URL, then it is quite important that you check that the URL looks correct, and you handle any exception that may result from the request. This allows you to provide valuable feedback to the user.

7
  • In an ASP.NET application from .NET 2.0 onwards, ASP.NET Health Monitoring will also log the complete exception, along with a lot of detail about what's going on. Jun 27, 2009 at 17:30
  • 3
    I disagree. It makes sense for a piece of code either to catch a general exception and change the Message to something more specific; or to check the preconditions. Yes, he could have permitted NullReferenceException to propagate, but that wouldn't have given the caller a clue that the exception was due to not being able to construct the connection. Jun 27, 2009 at 17:48
  • 3
    I disagree with John. A caller would indeed have a clue what caused the exception, because the exception includes the stack trace.
    – Jacob
    Jun 28, 2009 at 20:08
  • 1
    I agree with Jacob (to keep the linked list going). The sane thing to do is to have an AppDomain.UnexpectedException handler that dumps the exception.InnerException chain and all stack traces to a log file of some kind and then call Environment.FastFail. Jun 28, 2009 at 20:40
  • 3
    Exactly. The rule of thumb for exceptions is that if it's going to be thrown due to a config or coding error, DON'T catch it. If it's something that is occasionally expected to be encountered during user input, go ahead and catch it. The "defensive coding" example shown above is a "coding horror" indeed. Jun 28, 2009 at 20:48
13

Well, it depends who your audience is.

If you're writing library code that you expect to be used by a lot of other people, who won't be talking to you about how to use it, then it's not overkill. They'll appreciate your effort.

(That said, if you're doing that, I suggest you define better exceptions than just System.Exception, to make it easier for people who want to catch some of your exceptions but not others.)

But if you're just going to use it yourself (or you and your buddy), then obviously it's overkill, and probably hurts you in the end by making your code less readable.

7

I wish I could get my team to code like this. Most people dont even get the point of defensive programming. The best they do is to wrap the whole method in a try catch statement and let all exceptions be handled by the generic exception block!

Hats off to you Ian. I can understand your dilemma. I have been through the same myself. But what you did probably helped some developer several hours of keyboard bashing.

Remember, when you are using a .net framework API, what do you expect out of it? What seems natural? Do the same with your code.

I know it takes time. But then quality comes at a cost.

PS: You really dont have to handle all errors and throw a custom exception. Do remember that your method will be only used by other developers. They should be able to figure out common framework exceptions themselves. THAT is not worth the trouble.

5
  • 1
    Well, at least in an environment with exceptions the default fail state is "stop". If you were working in something like C, and your team ignored errors, your app would enter some undefined state and die at some unspecified point in the future.
    – Bernard
    Jun 27, 2009 at 17:34
  • I agree Bernard. I was talking specifically with respect to C#.
    – bobbyalex
    Jun 27, 2009 at 17:35
  • I know. That should read "at least you are", sorry.
    – Bernard
    Jun 27, 2009 at 17:37
  • I fail to see how how all the extra try/catch's are necessary to avoid "keyboard bashing". The stack trace shows where the exception originated and once you have that, solving the issue is trivial. Config and coding errors shouldn't be caught - let the framework do its thing. While I'm all for defensive coding, it's contextual and there is no value added here.
    – Cory House
    Aug 12, 2010 at 16:51
  • Cory, I agree with you completely and my point was just that. You dont have to handle ALL the exceptions, just the ones that make sense with respect to the current method. My point is that you shouldnt wrap all methods in a try catch and make the method swallow its own exceptions, some need to go through to the caller.
    – bobbyalex
    Aug 16, 2010 at 3:43
6

Your "before" example has the distinction of being clear and concise.

If something is wrong, an exception will be thrown eventually by the framework. If you can't do anything about the exception, you might as well let it propagate up the call stack.

There are times, however, when an exception is thrown deep inside the framework that really does not shed light on what the actual problem is. If your problem is that you don't have a valid connection string, but the framework throws an exception like "invalid use of null," then sometimes it is better to catch the exception and rethrow it with a message that is more meaningful.

I do check for null objects a lot, since I need an actual object to operate on, and if the object is empty the exception that is thrown will be oblique, to say the least. But I only check for null objects if I know that is what will occur. Some object factories do not return null objects; they throw an exception instead, and checking for null will be useless in these cases.

4
  • i originally had comments in the code, describing logically what's going on. i took them out so for this SO question to make it looks wonderfully concise, if uncommented :)
    – Ian Boyd
    Jun 27, 2009 at 20:12
  • 3
    The uncommented code is fairly obvious to anyone who understands the objects and their purpose. Jun 27, 2009 at 21:02
  • The point of comments is help the next guy to understand the objects and their purpose - and odds are that the next person will be me; 8 months from now. A note just before the first line saying something like "The static ConfigurationManager class pulls connection string info out of the web.config for us" really helps to explain what's going on. "Ohhhh, so they're stored in web.config? Oh yeah, that's right."
    – Ian Boyd
    Jun 28, 2009 at 4:03
  • I disagree Ian. Programmers shouldn't be expected to document the framework via comments - that's what framework documentation is for. Comments should say why you're doing something, and the code says what you're doing. Comments should not be a band-aid for incompetence in a technology.
    – Cory House
    Aug 12, 2010 at 16:56
3

I don't think I'd write any of that null-reference checking logic - at least, not the way you've done it.

My programs that get configuration settings from the application configuration file check all of those settings at startup. I usually build a static class to contain the settings and reference that class's properties (and not the ConfigurationManager) elsewhere in the application. There are two reasons for this.

First, if the application isn't configured properly, it's not going to work. I'd rather know this at the moment the program reads the configuration file than at some point in the future when I try to create a database connection.

Second, checking the validity of configuration shouldn't really be the concern of the objects that rely on the configuration. There's no sense in making yourself insert checks everywhere throughout your code if you've already performed those checks up front. (There are exceptions to this, certainly - for instance, long-running applications where you need to be able to change the configuration while the program is running and have those changes reflected in the program's behavior; in programs like this you'll need to go to the ConfigurationManager whenever you need a setting.)

I wouldn't do the null-reference checking on the GetFactory and CreateConnection calls, either. How would you write a test case to exercise that code? You can't, because you don't know how to make those methods return null - you don't even know that it's possible to make those methods return null. So you've replaced one problem - your program can throw a NullReferenceException under conditions that you don't understand - with another, more significant one: under those same mysterious conditions, your program will run code that you haven't tested.

1

My rule of thumbs is :

don't catch if the message of the thrown exception is relevant to the caller.

Thus, NullReferenceException don't have a relevant message, I would check if it's null and throw an exception with a better message. ConfigurationErrorException is relevant so I don't catch it.

The only exception is if the "contract" of GetConnection doesn't retrieve the connection string necessarily in a configuration file.

If it's the case GetConnection SHOULD have a contract with a custom exception which say that the connection couldn't be retrieved, then you can wrap ConfigurationErrorException in your custom exception.

The other solution is to specify that GetConnection cannot throw (but can return null), then you add an "ExceptionHandler" to your class.

1

Your method documentation is missing. ;-)

Each method has some defined input and output parameters and a defined resulting behaviour. In your case something like: "Returns valid open connection in case of success, else returns null (or throws an XXXException, as you like). Keeping this behaviour in mind, you can now decide how defensive you should program.

  • If your method should expose detailed information why and what failed, then do it like you did and check and catch all and everything and return the appropriate information.

  • But, if you are just interested in an open DBConnection or just null (or some user defined exception) on failure, then just wrap everything in a try/catch and return null (or some exception) on error, and the object else.

So I would say, it depends on the method's behaviour and expected output.

2
  • you would throw and not return the exception though, right?
    – Svish
    Jul 2, 2009 at 12:26
  • sure, exceptions should always be thrown (never returned one, yet)
    – MicSim
    Jul 2, 2009 at 12:43
1

In general, database-specific exceptions should be caught and re-thrown as something more general, like (hypothetical) DataAccessFailure exception. Higher-level code in most cases doesn't need to know that you are reading the data from the database.

Another reason to trap these errors quickly is that they often include some database details in their messages, like "No such table: ACCOUNTS_BLOCKED" or "User key invalid: 234234". If this propagates to the end user, it is bad in several ways:

  1. Confusing.
  2. Potential security breach.
  3. Embarrassing for the image of your company (imagine a client reading an error message with crude grammar).
2
  • 1
    You should not remove valuable information from exceptions! Rather, you should ensure that the top-level handler doesn't show the actual exception and stack trace to the end user.
    – JacquesB
    Jun 27, 2009 at 18:47
  • I'm not saying this should be removed, but it must be handled. And you don't want to catch SQLException in top-level code.
    – quant_dev
    Jun 28, 2009 at 13:03
0

I would have coded it exactly like your first attempt.

However, the user of that function would protect the connection object with a USING block.

I really don't like translating exceptions like your other versions do as it makes it very difficult to find out why it broke (Database down? Don't have permission to read configuration file, etc?).

2
  • Well if you look closely you can see that i didn't actually eat/handle any exceptions. In the end i just protected against two possible null reference exceptions.
    – Ian Boyd
    Jul 2, 2009 at 13:53
  • OK, you're right I didn't read it so well. I'm used to my own connection factory which can't return NULL.
    – Joshua
    Jul 2, 2009 at 15:36
0

The amended version doesn't add much value, so long as the application has an AppDomain.UnexpectedException handler that dumps the exception.InnerException chain and all stack traces to a log file of some kind (or even better, captures a minidump) and then call Environment.FailFast.

From that information, it will be reasonably straightforward to pinpoint what went wrong, without needing to complicate the method code with additional error checking.

Note that it is preferable to handle AppDomain.UnexpectedException and call Environment.FailFast instead of having a top-level try/catch (Exception x) because with the latter, the original cause of the problem will probably be obscured by further exceptions.

This is because if you catch an exception, any open finally blocks will execute, and will probably throw more exceptions, which will hide the original exception (or worse, they will delete files in an attempt to revert some state, possibly the wrong files, maybe even important files). You should never catch an exception that indicates an invalid program state that you don't know how to handle - even in a top level main function try/catch block. Handling AppDomain.UnexpectedException and calling Environment.FailFast is a different (and more desirable) kettle of fish, because it stops finally blocks from running, and if you're trying to halt your program and log some helpful information without doing further damage, you definitely don't want to run your finally blocks.

0

Don't forget to check for OutOfMemoryExceptions... you know, it might happen.

2
  • 1
    I hope this is sarcasm! The problem is, someone might see this and take it seriously. (Fortunately in CLR 4.0 they won't be able to catch such exceptions). Jun 28, 2009 at 20:53
  • Yes it's sarcasm maybe I need to clarify :)
    – dr. evil
    Jun 29, 2009 at 8:08
0

Iain's changes look sensible to me.

If I am using a system and I use it improperly, I want the maximum information on the misuse. E.g. if I forget to insert some values into a configuration before calling a method, I want an InvalidOperationException with a message detailing my error, not a KeyNotFoundException / NullReferenceException.

It's all about context IMO. I've seen some fairly impenetrable exception messages in my time, but other times the default exception coming from the framework is perfectly fine.

In general, I think it's better to err on the side of caution, especially when you're writing something that is heavily used by other people or typically deep in the call graph where the error is harder to diagnose.

I always try to remember that as a developer of a piece of code or system, I am in a better position to diagnose failures than someone who is just using it. It's sometimes the case that a few lines of checking code + a custom exception message can cumulatively save hours of debugging time (and also make your own life easier as you don't get pulled around to someone else's machine to debug their problem).

0

In my eyes, your "after" sample is not really defensive. Because defensive would be to check the parts under your control, which would be the argument connectionName (check for null or empty, and throw an ArgumentNullException).

0

Why not split up the method you have after you added all the defensive programming? You have a bunch of distinct logic blocks which warrant separate methods. Why? Because then you encapsulate the logic that belongs together, and your resulting public method just wires those blocks in the right way.

Something like this (edited in the SO editor, so no syntax/compiler checks. Might not compile ;-))

private string GetConnectionString(String connectionName)
{

   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
   if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");
   return cs;
}

private DbProviderFactory GetFactory(String ProviderName)
{
   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+ProviderName+"\"");
   return factory;
}

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs = GetConnectionString(connectionName);

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = GetFactory(cs.ProviderName);


   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

PS: This is not a complete refactor, only did the first two blocks.

1
  • i much prefer the self-contained logic.
    – Ian Boyd
    Jul 2, 2009 at 12:26

Not the answer you're looking for? Browse other questions tagged or ask your own question.