Returning a Flag Indicating Success

Question

Whenever I'm writing code, I always stub out my methods like this (not necessarily using generics):

public T MyMethod()
{
    T result = default(T); // or null

    return result;
}

This always puts me in the right place to fill out the logic and make sure that I have a return variable to track throughout the process. After having done this long enough I've found a development pattern arose, and I'm not sure which direction to take. Usually in some relatively trivial operation:

public bool AddRecord(Record r)
{
    bool result = false;

    if(_collection != null)
    {
        _collection.Add(r);

        result = true;
    }

    return result;
}

As an aside, I'd generally assign the result of the inner callee to result but some methods (such as Add) don't have any result.

This method, as it exists, is fine. But for some systems, I would write it a little different.

public bool AddRecord(Record r)
{
    bool result = false;

    if(_collection == null)
    {
        _collection = new List<Record>();
    }

    _collection.Add(r);

    result = true;

    return result;
}

And it's here that I ponder what I should do. Always returning true provides no value to the end user, in my opinion. But then I say, "Well if, in the future, this method grows in complexity, would you want to have to change the method signature and have all callers update their reaction to this? Or would you rather just return true for now?" to which I tend to favor the latter, unless it's something so trivially straightforward that I have high confidence it will not change (which, in itself, has ~50% chance of being wrong). Another option would be to assume that, although this interior logic can't generate a false-y condition, it could throw an exception. Should my method handle that and return a flag indicating whether an exception was thrown? Thinking about this, I think:

• Eating exceptions is bad, and unless I can do something with the information given to me, I shouldn't do it and I should let the caller handle it.
• Does the caller even care about this exception, or do they only care whether this operation succeeded as a whole?

This leads me to this state where I have a method in which I'm not sure what to do, so I typically return a constant true and do not eat the exception.

My question is: In the general case, what is the best way to approach this?

I understand that specific cases require specific consideration, and while writing my code I always consider the unique case each method presents. But when I realize a method is not any more unique than another, I'd like have some additional insight as to what I should do. Many thanks in advance for your opinion.

Answer 1

In my opinion, methods should either return a value or have a side effect, but not both.

In this answer to another question the author quotes the source of that practice.

In your examples, you handle a situation in which you try to add an element to a collection that could be or could not be initialized. My suggestion is that you either throw an exception if the collection is not initialized, or initialize it if it is null and then add the item, but don't return a value.

In this answer to yet another question you can see the disadvantages of return codes as compared to exceptions.

To summarize it:

• Return codes are more verbose
• Return codes are more brittle
• Return Codes must sometimes be translated
• Return Codes means you can't chain expressions
• Exception are typed
• Exception are... synchronous

Answer 2

You're right that it's generally best to let other code's exceptions propagate through yours unimpeded. Where I work, this is called being "exception-agnostic".

Now for the hard part. The return values of your functions are part of your public API. Having a return value that doesn't mean anything is a lot like having a method that doesn't do anything when called. It just shouldn't be there.

More importantly, let's say you did publish a function that always returned true because it could not fail. Then one day you change it so it can fail, and the function sometimes returns false. You may not have changed the function signature, but you have changed the contract. As a result, all existing code using that function will probably be broken, because it was written against the old "this function cannot fail" contract. That could be much worse than simply failing to compile.

So make the signature match the contract. If it can't fail, don't return a success/failure value. Anything else merely causes confusion, or fear that you might make backwards-incompatible changes someday. When you need to make a massive semantic change like introducing the possibility of failure, that needs to go into a brand new function.

Incidentally, when some methods can fail in a variety of meaningfully different ways, it's often a good idea to group them together into one method so you can return a single enum describing every possible kind of failure. A typical example would be the executeQuery() function for a database library.

P.S. I think AddRecord would be more readable with an early return (if(_collection == null) { return false; }) but maybe that's just me.

Answer 3

If I came across a method called AddRecord, I'd conclude - from it's name - that it was a statement, not a yes/no question. It's not asking me if I want to add a record; I'm telling it to do so. Therefore I'd expect one of two possible return types, depending on whether I'm thinking OO or functional:

• void - I've commanded it to do something. If there's a problem, it'll throw an exception.
• ICollectionOfSomeSort - I've asked my collection to be added to, so I'll expect a collection back (ideally a brand new read-only one).

Getting a boolean back doesn't make much sense. A true might reassure me that it worked, but what does false mean? "Couldn't be bothered"? "Sorry, I don't add to collections on a Friday"? No failure reason is provided; just a "computer says no" response.

However, care needs to be given to when and why an exception would be thrown. In various examples, you are testing the state of _collection. In one case you return false if it's null. Other answers have suggested you should throw an exception in this case. This implies that it's the caller's responsibility to initialise the internal state of your class before calling AddRecord and that you'll fail the call if they don't. This is a leaky abstraction: you are exposing the inner workings of your class through these failure messages. Any internal setup like this should happen without the need for the API user to initiate it. If a NullReferenceException occurs, it should indicate a bug in the code, ie it should only ever be your fault (as the API creator), rather than mine (as the API user).