12

I see the following code pattern all over the place in my company's codebase (.NET 3.5 application):

bool Foo(int barID, out Baz bazObject) { 
    try { 
            // do stuff
            bazObject = someResponseObject;

            return true;
    }
    catch (Exception ex) { 
        // log error
        return false;
    }
}

// calling code
BazObject baz = new BazObject();
fooObject.Foo(barID, out baz);

if (baz != null) { 
    // do stuff with baz
}

I'm trying to wrap my head around why you would do this instead of having the Foo method simply take the ID and return a Baz object, instead of returning a value that isn't used and having the actual object be a ref or output parameter.

Is there some hidden benefit of this coding style that I'm missing?

yannis
  • 39,547
  • 40
  • 183
  • 216
Wayne Molina
  • 15,644
  • 10
  • 56
  • 87
  • Related thread: [When and why you should use void (instead of i.e. bool/int)](http://programmers.stackexchange.com/questions/67707/when-and-why-you-should-use-void-instead-of-i-e-bool-int) – Péter Török May 13 '11 at 14:10
  • In your example, `baz` being `null` and the returned `bool` being `false` are *not* equivalent. `new BazObject()` is never `null`, so unless `bazObject` is updated before the `Exception` is thrown in `Foo`, when `false` is returned `baz` will never be `null`. It would help tremendously if the *spec* for `Foo` were available. In fact, this is perhaps the most serious problem exhibited by this code. – Steve Powell Aug 21 '14 at 14:38
  • My memory is hazy as this was a long time ago but I think that I had messed up the example and it was checking if "baz" was false, not if it was null. In any event the pattern just seemed really archaic to me, like it was from VB6 and the developer never bothered to improve his code (which he didn't) – Wayne Molina Aug 21 '14 at 21:53

5 Answers5

12

You usually use that pattern so you can write code like this:

if (Foo(barId, out bazObject))
{
  //DoStuff with bazobject
}

it's used in the CLR for TryGetValue in the dictionary class for instance. It avoids some redundancy but out and ref parameters always seemed a bit messy to me

Homde
  • 11,104
  • 3
  • 40
  • 68
  • 1
    +1, this is the reason why, though I tend towards returning an object with both the boolean and the object rather than returning them both separately – pdr May 13 '11 at 14:02
  • Going to mark this as the answer since it does explain the reason, although the consensus seems to be it's pretty outdated except in specific circumstances. – Wayne Molina May 13 '11 at 14:07
  • This answer justifies the use of this pattern. But if it is ALL OVER your codebase, it's probably a bad practice like Sean side. – Codism May 13 '11 at 14:30
11

This is old C style code, before there were exceptions. The return value indicated whether the method was successful or not, and if it was, the parameter would be filled with the result.

In .net we have exceptions for that purpose. There should be no reason to follow this pattern.

[edit] There are obviously performance implications in exception handling. Maybe that has something to do with it. However, in that code snippet there's already an exception being thrown. It would be cleaner to just let it move up the stack until it's caught in a more appropriate place.

Sean Edwards
  • 449
  • 2
  • 7
  • No. Can't agree with that. Never use an exception for an expected condition. If you're parsing a string to an int, for example, it is always possible someone will pass an unparsable string. You should not throw an exception in that case – pdr May 13 '11 at 13:59
  • That's not what I said. If you read the code that OP posted, there is explicitly an exception case happening, but the method catches it and returns false instead. This question is related to error handling, not expected conditions. – Sean Edwards May 13 '11 at 14:01
  • Yeah, it seems to be mainly used in methods that load data from the database e.g. `if (baz.Select())` but more often than not the return value is just thrown away and the value is checked against null or some property. – Wayne Molina May 13 '11 at 14:02
  • @Sean, see what you're saying, I just object to the phrase "In .NET we have exceptions for that purpose." Exceptions serve an entirely different purpose for me – pdr May 13 '11 at 14:05
  • @pdr, if you can't use an exception to handle invalid input then you might as well not use exceptions at all. Handling invalid input is an ideal case for exceptions. – edA-qa mort-ora-y May 13 '11 at 14:06
  • 1
    Ok, let me be clear. It's true that you shouldn't just add a boolean to every method to account for coding errors on invalid arguments. But *where the input to a method comes from a user rather than a developer* you should be able to try to handle the input without throwing an exception if they got it wrong. – pdr May 13 '11 at 14:13
  • I want to add that this paradigm isn't just for invalid arguments. A failing case may be perfectly acceptable, for example the argument may be optional. When you TryParse user input, it may be ok for them to put in a number or something else. Throwing exceptions there is just wasteful. – Tesserex May 13 '11 at 14:17
2

Given the code snippet it looks completely pointless. To me the initial code pattern would suggest that null for BazObject would be an acceptable case and the bool return is a measure of determining a fail case safely. If the following code was:

// calling code
BazObject baz = new BazObject();
bool result = fooObject.Foo(barID, out baz);

if (result) { 
    // do stuff with baz
    // where baz may be 
    // null without a 
    // thrown exception
}

This would make more sense to me to do it this way. Perhaps this is a method someone before you was using to guarantee that baz was being passed by reference without understanding how object parameters actually work in C#.

Joel Etherton
  • 11,674
  • 6
  • 45
  • 55
1

Sometimes this pattern is useful when you, the caller, aren't supposed to care whether the returned type is a reference or value type. If you are calling such a method to retrieve a value type, that value type will either need an established invalid value (e.g. double.NaN), or you need some other way of determining success.

Kevin Hsu
  • 1,613
  • 10
  • 11
0

The idea is to return a value that indicates whether the process was successful, allowing code like this:

Baz b;
if (fooObject.foo(id, out b)) {
   // do something with b
}
else {
   Error.screamAndRun("object lookup/whatever failed! AAaAAAH!");
}

If the object cannot be null (which appears correct in your example), it's better done as follows:

Baz b = fooObject.foo(id);
if (b != null) {
   // do something with b
}
else {
   Error.screamAndRun("object lookup/whatever failed! AAaAAAH!");
}

If the object can be null, exceptions are the way to go:

try {
   Baz b = fooObject.foo(id);
}
catch (BazException e) {
   Error.screamAndRun("object lookup/whatever failed! AAaAAAH!");
}

It's a common pattern used in C, where there are no exceptions. It allows the function to return an error condition. Exceptions are generally a much cleaner solution.

Michael K
  • 15,539
  • 9
  • 61
  • 93