2

I've seen this sort of pattern in code before:

//pseudo C# code

var exInfo = null;  //Line A

try
{
  var p = SomeProperty;         //Line B
  exInfo = new ExceptionMessage("The property was " + p);           //Line C
}
catch(Exception ex)
{
  exInfo.SomeOtherProperty = SomeOtherValue;  //Line D
}

Usually the code is structured in this fashion because exInfo has to be visible outside of the try clause. The problem is that if an exception occurs on Line B, then exInfo will be null at Line D. The issue arises when something happens on Line B that must occur before exInfo is constructed. But if I set exInfo to a new Object at line A then memory may get leaked at Line C (due to "new"-ing the object there). Is there a better pattern for handling this sort of code? Is there a name for this sort of initialization pattern?

By the way I know I could check for exInfo == null before line D but that seems a bit clumsy and I'm looking for a better approach.

5
  • This code looks like a faulty strange code.
    – BenjaminB
    Commented Oct 17, 2012 at 20:13
  • What is a "faulty strange code"? I did remove some of the context to try to pare down to the essence of the question but I assure you this is not a contrived example. Commented Oct 17, 2012 at 20:54
  • It's an error to use exInfo without being sure it's not null. Of course this is a bad pattern, there is no argument possible on this.
    – BenjaminB
    Commented Oct 17, 2012 at 21:13
  • Seems to me like this question is a classic case of attempting to overdesign.
    – Dunk
    Commented Oct 17, 2012 at 22:50
  • @Dunk would you mind elaborating on your comment? I fail to see how this is overdesigning anything. It's existing code and I've seen the pattern in other places; I was just trying to find a more general solution to the problem. Commented Oct 19, 2012 at 15:41

4 Answers 4

5

You should initialize exInfo before the try block. In .NET you won't leak any memory at Line C.

I'm going to try and make a concrete example out of your pseudocode, and see if that helps. Assume db.getList() runs some SQL and returns NULL if nothing found, a list if elements were found, and throws an exception on error.

List<String> stuff = new ArrayList<String>();

try {
   stuff = db.getList("SELECT someColumn FROM someTable");
}
catch (SqlException e) {
   log.error("Couldn't retrieve stuff!", e);
}

return stuff;

In Java and other garbage-collected languages, no memory would be leaked after your line C if you initialized the variable previously. The initial object would have no references associated with it and would be then eligible for garbage collection. This pattern allows you to keep code calling this code null-safe, since it will always return a valid List<>.

4
  • Thanks--this is exactly the sort of information I was seeking. Commented Oct 17, 2012 at 20:06
  • So how do you know that the query failed versus there was no matching data using this approach?
    – Dunk
    Commented Oct 17, 2012 at 22:54
  • @Dunk In many cases, I don't think you want a SQL error to crash the application. In our case, the most likely explanation for SQL failing in production is that the DB connection went down, and monitoring would detect that. We also monitor the ERROR level logging. So, fail soft (still logging the exception so we can see precisely what happened) and return an empty list so that upstream code doesn't need to null-check.
    – Michael K
    Commented Oct 18, 2012 at 12:34
  • And you don't have to handle the error as I did here...the general pattern is only to prevent returning a null list. You may need a different contract that allows null in case of errors. In that case this pattern wouldn't be useful.
    – Michael K
    Commented Oct 18, 2012 at 12:36
4

It's a good idea for a variable to never contain an incorrect value, if at all possible. That way there's no possible way, for example, for maintainers to later insert a bunch of statements between Line A and the try block, then somewhere in there accidentally use exInfo when it's null. The general solution when you're tempted to initialize a value to null due to scope reasons is to break off the initialization code into its own function, so you would call:

var exInfo = createExInfo();

And createExInfo would contain:

try
{
  var p = SomeProperty;
  return new ExceptionMessage("The property was " + p);
}
catch(Exception ex)
{
  var exInfo = new ExceptionMessage();
  exInfo.SomeOtherProperty = SomeOtherValue;
  return exInfo;
}
1
  • Excellent suggestions Karl. Commented Oct 18, 2012 at 13:16
1

I would refactor like this:

        var p = null;
        var exInfo = null;

        try
        {
            p = SomeProperty;         //Line B
        }
        catch (Exception ex)
        {
            exInfo = new ExceptionMessage("The property was " + p == null ? "foo" : "bar");           //Line C
            exInfo.SomeOtherProperty = SomeOtherValue;  //Line D
        }

        exInfo.BlahBlahBlah = "Blah";
0

How about creating class that has an event handler? Then initialize exInfo before initializing var p, which would guarantee that exInfo would not be null?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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