A function that returns only true

Question

During a code review I performed today for my colleague, I noticed a function that was defined as returning a boolean value, but in practice it returned only true. In a case of failure, this function threw an exception. I pointed it out and advised to change the return value to void (the code is written in C++). I had no doubt that this was wrong and was sure he had just overlooked it. To my complete surprise, the programmer told me that it was intentional for forward compatibility; he told: "What if we decide later that the function should return false and not throw an exception? The code then will just neglect the return values at all." After that we had a pretty heated dispute that ended with both sides staying at the same positions - he refused to change the function.

To have the complete picture, the purpose of the function is to initialize a networking module. A failure in this case is critical - the application halts if the module cannot initialize. But it isn't about returning false or throwing an exception. The man argued that even if the function didn't throw and always succeeded, he would have made it with boolean return value just for that forward compatibility.

Also, I should add, that we don't throw exceptions to return error codes from functions, but use them as a sign of a completely catastrophic failure. That is, there are no try-catch blocks except for the main function. Exceptions in our case are just an alternative to calling exit(errno). From this point of view my colleague was correct - his function always succeeds or completely shuts down the process.

I still believe that I am right and that such a function is a bad and even dangerous coding style, but it made me think, maybe it's just me? Maybe I'm too stuck in my personal dogma disconnected from the reality. What do you think? Is such a function an error, a weird but acceptable style, or a matter of personal preference?

Answer 1

Tell him that it should return void * in case you decide later it should return an integer indicating the number of errors, or a string describing the error, or an array of error information objects....

Answer 2

I think you're right, either you signal errors with a return value or you throw an exception, not both.

Answer 3

If I were going to change a function so much that its return type changed, I'd probably just write a new function. It sounds like your friend needs to be acquainted with YAGNI.

Answer 4

What if we decide later that the function should return false and not throw an exception? The code then will just neglect the return values at all

Actually, I think this reasoning is bad. The decision that a function should throw an exception on error, as opposed to returning an error value, should be fixed once the API is implemented. This way (leaving the return type bool just in case) is not testable when it returns always true, and doesn't actually enforce the client checks the value. It is only a source of ambiguity.

Answer 5

The main problem with returning bool (in this case always true), is that down the road a programmer might depend on the function returning false, not knowing that it could never happen. Its really a violation of the black box principle- you should not necessarily have to know how an api is implemented, just how to use it. Since the function presents that it can return either true or false, but doesn't live up to its promise, its broken.

Answer 6

What if we decide later that the function should return false and not throw an exception? The code then will just neglect the return values at all.

Then the calling code should be rewritten anyway. Otherwise, it will just work incorrectly: when a failure happens, the calling code would not catch an exception, ignore result, which is false, and lead to unexpected consequences.

The situation when calling code should be changed to retain old behavior means that there is no forward compatibility. Hence there's no reason for the function to return bool.

Answer 7

+1 Bill The Lizard

1. We should not break backward compatibility - It's a nightmare for someone using your api

2. Error codes are always helpful

You should write a new function in case you want to support error codes and deprecate the old api. This makes you and everyone happy :)

Answer 8

The only reason I can see for such a function to return a Boolean variable would be if there were some realistic possibility of it returning a useful piece of information other than success or failure; even if the present implementation always returns True, it may be reasonable to return a Boolean if the Boolean indicates something that is true of the present version that may not be true of future ones. If the Boolean represents something that might be interpreted as part of the current routine's contract, unit tests should mock up a version that returns false to ensure sensible handling.

For example, a "commit transaction" routine might return a Boolean to indicate whether the transaction has been made immediately visible to everyone else (True), or whether the database has made the commit inevitable (so that even in case of power loss, the commit would be completed on next startup) but not yet visible to other clients. If the commit occurs but its effects will not be immediately visible, the application might want to know that, but the delayed visibility wouldn't be an "error". On the other hand, if the contract for the "commit transaction" routine implies immediate visibility, a new routine which could return while the commit was still being processed would violate that contract.

Answer 9

Is this how your friend wants to use the function?

try {
 ret = the_function();
 if(ret == false) { // for the sake of "forward compatibility"
     //handle it
 }     
} catch some_exception { 
  // handle it
} 

This is ugly at best. And if he doesnt want those ret==false checks, theres any way no point having the boolean return value as you would have to make those changes anyway.

I'd not mind a boolean value for a function that I know always returns true when i've decided to not use exceptions at all.
To move from exceptions to error codes or the other way around is anyway a lot of code change. There is no point in that forward compatibility.

Answer 10

Sounds to me like you're both standing on the same middle ground. By this I mean that the way the code is now is not acceptable. You both have alternative proposals on how to change it: you say set it void and let the error break the code flow, he says it may someday return false instead of an error.

I agree with you in the fact that it should not be left alone, but I agree more with his change, having it return true or false, and no exception. The way it is now is not a good development practice. What if, by some off chance (maybe a developer mistake), you do return false without setting off an exception? What then?

If I were to make a recommendation, I would say that it returns true on success, false on any possibly foreseeable/fixable errors, and throw an exception on unforeseen errors.

Answer 11

My argument:

Can you successfully write a test-case that tests this function when it returns false?

If you can't test it, you aren't allowed to code it.

Since there is no test-case that will result in returning false, you aren't allowed to code it that way.

Answer 12

Focusing on the main argument for keeping the return value: "What if we decide later that the function should return false and not throw an exception? The code then will just neglect the return values at all."

You can't be sure the code isn't already neglecting the return values somewhere. Since an exception is thrown and a false is never returned, this means the code that should react to a failure as a return value is never executed, and so was never tested. If returning a value will not guarantee it will be tested in the future, then it does not guarantee the same right now.

In the future, if you choose to change the behavior, the best choice is to write a new method with a different name that makes clear the new behavior - and to gradually deprecate the old one.

For instance, if the current method is openConnection, the new should be tryOpenConnection.

Answer 13

Either use an exception or a return code. Not both. Ideally if your function throws an exception, there should be a way of determining ahead of time whether the function will throw an exception.

// Return value
if (this->Initialise())
{
    // Carry on
}
else
{
    // Panic stations!
}

// Exceptions
try
{
    this->Initialise();
    // Carry on
}
catch (Exception &exception)
{
    // Panic stations!
}

Answer 14

For reasons of forward compatibility I believe all functions should return 42. Of what possible significance could the question be when you already know the answer?

Edit

On a more serious note, the use of the word forward here is just a synonym for premature. By extention: forward compatibility == premature compatibility. This belongs in the same tool set family as premature optimizations. Enough said.

A Code Review should never degrade into a debate. Issues, like this one, should be noted by the moderator allowing the review process to move forward. The risk here is that time spend debating side-line issues is not being used to identify more significant defects.

Answer 15

You may want to look at Java's Collection<>.add() method. Here's a link.

Collection (Java 2 Platform SE v1.4.2)

Answer 16

If it should be forward compatible, then all code that currently, and in the future, uses this function must correctly handle the case where it returns false, otherwise there is no forward compatibility. So, it should be tested by hard coding a return value of false in it (could be done with a compiler directive, a code change, whatever) and testing that all calling functions correctly handle the situation where it returns false rather than throwing an exception.

Otherwise, there is no forward compatibility and the code is incorrect.

I would be fine with it IF he does test every calling function and ensures that it handles the false case correctly, and that this is done for any changes in code that calls it. It's his time to waste. If someone else has to test that (probably 90% probability) then it's a waste of other people's time too and a bad idea.

Personally, I would feel that time is better spent elsewhere, than testing code twice for a scenario which won't happen unless something changes in the future. It is basically a bad idea.

Answer 17

Your colleague is wrong, but I can understand his point. Initialization functions traditionally return true if the init is successful, false if not. When Exceptions appeared, returning a true/false status became redundant, as you have a different channel when propagation is done. So, at first glance, this function should be void.

However, there is a case where false can be returned, namely when you try to reinit, if the reinit does nothing. This is an ugly condition, potentially indicates a bug, but throwing an exception is a bit too harsh. Ok, you are calling an init because you must to. If you call init another time, it's technically not a mistake, you are just reinitializing, but if you already called it and the call does nothing, it makes sense to return false to communicate this fact, if needed.

So, the point raised by your colleague is wrong, but the fact is that there is already a good reason to return false, and it should be implemented right now.

Answer 18

There sounds as if there are two issues going on here. First being which approach to take for the return value of this function, and the second being the handling of the disagreement over said approach. Plenty of people have chimed in on what/how the function has returned so I'd like to address what happened in the code review.

After that we had a pretty heated dispute that ended with both sides staying at the same positions - he refused to change the function.

Clearly, this is suboptimal, and I'm going to wildly speculate that this result stemmed from two things: 1) the benefits between the different implementations are limited, and 2) you were both for what ever reason being stubborn.

First:
When reviewing the two alternatives for the function signature I do actually agree that the void return is better than a bool that's always true, but is the advantage of your approach really worth it? While yes there is an advantage, it's not an O(N) difference in a fundemental algorithm. You have to know when to pick your battles. Was/is this one worth it?

Second:
As for your coworker, the same could be said for him. Both of you stood your ground and refused to budge despite alternatives you could have taken such as asking for a third set of eyes, speaking to the other devs to see what the consensus was between the two styles, or just accepting that maybe the other person is unrealistically attached to their idea of how it should be done and walking away from it. You will meet plenty of people in your career who are more concerned with being right than with what's best so sometimes walking away is really the best approach. Again, you have to know when to pick your battles.

Answer 19

A method should do one thing and only one thing.

A method that returns boolean should be named something that indicates it asks a question.

For example, isOrderFilled or hasABlueTint

A method that processes something should be aptly named to indicate what it does. A method should not both process something and answer a question.

The way I would win this argument is to look at the heart of the problem, which is the method is doing too much.

A fair compromise would be to have the object have one method that performs the action and another method that can be called to check for error states.

Answer 20

Actually your colleague postpone any design about the code.
The usage of code like this, is good way to create bugs.

Answer 21

If this is a sign of larger coding trends, and it sounds like it may be, then I think it may deserve the degree of discussion that you 2 gave it.

If it's a one time thing, I'd say that 2 good and probably high-paid programmers may have spent a good deal of time, and hence: money, attending to a trivial matter instead of being productive.

Answer 22

The return depends on the significance of the function. Not really forward compatibility matters. The software must have a good error handling mechanism, in the old days with "C" we rely on return types or some global strings to represent the error number/string. but the object oriented programmers mostly support with exception handling. I think with C++, not really everyone properly using exception handling.

Failing initialization is a catastrophic error. Error handling should be part of your software architecture. So it must be defined and followed by everyone working for the same project.

I agree with your view point on changing to void. at the same time, a function like Initialize should always return TRUE doesn't make any sense. If the Initialize function doesn't make much sense in your context, just don't care about the return value or throwing exception.

As someone else said here, keep it simple that's the best thing we can do with this

Answer 23

Another typical "what-if" dilemma. If that's the case (return boolean for future compatibility), there would not be any void method, isn't it?

It might be confusing for other programmers to call the method and wonder what is going to be returned, and what the meaning of true/false is.

As mentioned by others, follow YAGNI and KISS. Change the coding only when you really need it.

Answer 24

Surely your answer depends on how soon this forward compatibility is required. If you're writing the code next week, he's right. If there's no plans to add anything soon, you're right.

Answer 25

A note on:

"forward compatibility"

While the discussion is already half a day old, let's add a little mind exercise for your co-worker. Currently, we have only true, or an exception. In the future, he says, there may be true and false (and still an exception?). Maybe later, you need a tristate: true, unknown, false. Yet later, you want to return a class that holds the original error code, a success/fail boolean and a message string of the original error.

How would your co-worker ever anticipate every future scenario? Answer: he cannot and he should not. If you don't need it now, don't write it. If you need it later, refactor or write a different method.

To illustrate this, to understand the code currently written, the documentation must say something like the following:

/** 
    Connects to a server and returns true on success
    returns    true on success
    throws     if connection fails
    remarks    currently doesn't return false, ever, but may return false  
               in a future version, so make sure you check the return value 
               in case it ever changes
**/

So, your co-worker basically asks every developer to also become future-proof for their code, adding to the spaghetti that he already wrote, adding to the complexity of the overall system and the untestability of never-hit branches. If they don't do this, his future-proofness is worthless. If they do, they will be annoyed each time they call his function and have to add template code that could've been avoided. This will exponentionally grow when more dependencies arise, so it's high time you stop this co-worker and put some sense under his skull.

Need I say more? YAGNI was already mentioned. Code smell perhaps too. It's all there. I just wanted to illustrate how wrong it can be to code too much.

Answer 26

The whole point of this (exception, return code) is to let me, the user, to handle the failure, either through an exception catch, or an else clause in the if.

As a user, I thus expect your function to be correctly documented.

If it says it throws an exception on failure (and I hope you did document that), then I will try/catch the code if needed, and don't bother if I don't want to deal if the exception, letting the "higher code in the stack" deal with it.

If it says it returns false on failure, then I must handle the failure, or manually propagate it to let the "higher code in the stack" deal with it.

The two ways are incompatible, and if you change the interface of your function, you will anger me, as a user, because I will have to rewrite the code.

This means:

• your colleague is naive/misguided because he/she believes he/she can just remove the exception, return some bool, and hope my code will cope with it. Be sure it won't: There's no way I'll write two error handlers (one try/catch, one if/else) just because your colleague wants to appear proactive.
• Don't change the interface of the function, unless you want me to hate you. Which means this function will always throw an exception on failure. the boolean return is useless in this case, but won't kill any kitten, so...
• If you want to change the error handling, then keep the function as it is, and keep ascendant compatibility, write another function, which returns a boolean and don't throw an exception.

Answer 27

Refer your code review to a "3rd" pair of eyes; after that, write or append your decision to your coding guidelines.

But if it was me reviewing and/or approving the code, I would make the developer change his code to remove the boolean return value.

There are enough problems with backward compatibility, no need to add potential forward compatibility issues that will lead to confused code and confused developers.

M.

Answer 28

I suppose, if this is the only problem you found in the code, you have reason to be happy and not upset, and your colleague is a genius. In that case, I'd allow him to carry on with this rather strange style.

I think it'd make sense to return false and throw the exception within the calling code, if false is returned, until you write code to handle it. I think it introduces a weird sort of dependency, because if you decide, you want to handle failure, then you do not only have to write the code to handle the problem, but also alter the function itself. I don't think it is the responsability of the function or even the containing module to shut down the application. The one resposible is the calling class, thus it should decide, whether it can handle the failure or not, and raise an exception accordingly. The actual problem is, where the exceptions are thrown, not that something is returned (which should rather be a code and not a flag), which is why I think both none of you is right and both the current design and your proposition have the same dependency issue.

But really, I think this particular problem is nothing to get angry about. It only leads to frustration. It is not the point of code review to start religious wars over things like these.

Answer 29

I think , as long as the application program is working without any bugs it's OK but if you plan to make modifications later then you must be prepared for that. And common coding style eliminates confusion at later stages.

Answer 30

My take on this: C++ exceptions are for handling exceptions, not for handling "frequent errors". This is a fundamental conceptual difference. Also, handling exceptions is more costly, even when no exception occurs. Use both, but for different purposes, clearly defined up front.

Answer 31

I would ask "what is the meaning of the return value". For example what would be the name of the variable the caller would assign it to. This would show clearly whether the future proof argument makes sense. He is claiming that he is establishing a semantic contract. What is it?

If you are a exception throwing dev team (and it looks like you are) then having a return value meaning 'succeeded' is plain wrong. A reasonable caller cannot usefully use the return value for error handling and so he will not test it; he will ignore the return value. SO once again his argument is wrong

Having said all that; who cares? You have code reviews - that puts u in the elite. Its a minor point that sounds more like a head-butting dispute than a real issue

Answer 32

If you change the meaning of the existing function in the future, then IMO you can (or should) rename it. In fact you can keep the old function (which throws exceptions), and implement a new one with a different name (which returns boolean).

Answer 33

You can find functions which return only one value in serious software. It should be a matter of design and not a foresight of what will always happen or forward compatibility.

Answer 34

From that you write, I think that the module function should not thrown a fatal exception. If the application cannot work without this module initialized, the fatal exception should be thrown by the application itself.

In this regard your colleague is correct, i.e. preparing the function interface for a more correct module design.

I bet that throwing an exception inside this function is not an idea of your colleague, but a bad hack introduced by someone other :)

Answer 35

Unless the function is part of an external API, it will not matter at all, since the return value is thrown away immediately, anyway. Any future change in this behaviour will mean that both the calling as well as the called function will have to be rewritten and recompiled anyway. I would point this out, but if he insists, so what? He can return a Hopplewoop object for all you care, as long as it is destroyed properly.

Answer 36

As a side note your co-worker is justified in the following scenario.

You are overriding a framework class method that returns a boolean. You want to fail fast if the result is false. i.e you want to throw the exception.

Example: In Asp.Net VirtualPathProvider has a FileExists method. And you are providing your own VirtualPathProvider (to serve files from virual file system) and you don't want to continue any further if the file doesn't exist and let the user know about the issue immediately.

Answer 37

I can't see that it's been mentioned, but the "future" compatibility is already there with result type void. E.g., once in the future, given the throwing-on-failure ...

    void initNetworkModule() { ... }

... if one wants a boolean-valued function (perhaps in order to not terminate the app, under the general rule that the OP sketches where any propagated exception causes termination), well it's no big deal to define a non-throwing wrapper, say, initNetworkModuleSucceeded.