views:

1338

answers:

14

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?

+27  A: 

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.

JacquesB
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.
John Saunders
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.
John Saunders
I'm surprised this is the top rated answer. I agree with John Saunders.
CiscoIPPhone
I disagree with John. A caller would indeed have a clue what caused the exception, because the exception includes the stack trace.
Jacob
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.
Daniel Earwicker
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.
Dave Markle
@Dave: What about it is a coding horror, did i catch any exceptions?
Ian Boyd
+7  A: 

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 of 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.

Bobby Alexander
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
I agree Bernard.I was talking specifically with respect to C#.
Bobby Alexander
I know. That should read "at least you are", sorry.
Bernard
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
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.
Bobby Alexander
+12  A: 

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.

mquander
+6  A: 

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.

Robert Harvey
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
The uncommented code is fairly obvious to anyone who understands the objects and their purpose.
Robert Harvey
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
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
+1  A: 

Your method documentation is missing. ;-)

Each method has some defined in- 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 to 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 appropiate 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.

MicSim
you would throw and not return the exception though, right?
Svish
sure, exceptions should always be thrown (never returned one, yet)
MicSim
+1  A: 

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. embarassing for the image of your company (imagine a client reading an error message with crude grammar)
quant_dev
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
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
+2  A: 

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.

Robert Rossney
A: 

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?).

Joshua
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
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
+1  A: 

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.

Nicolas Dorier
A: 

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.

Daniel Earwicker
A: 

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

dr. evil
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).
Daniel Earwicker
Yes it's sarcasm maybe I need to clarify :)
dr. evil
A: 

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).

Mark Simpson
A: 

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).

Lucero
A: 

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.

Erik van Brakel
i much prefer the self-contained logic.
Ian Boyd