How defensively should I program? [closed]

Question

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?

Answer 1

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.

Answer 2

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.