views:

6059

answers:

38

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?

+134  A: 

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....

JoelFan
Then it should return `template <typename T> T` :p
KennyTM
favorited just for this answer ><
Samuel_xL
...and the whole code should be refactored to return only that and all arguments should be variable length parametric templates and actually the whole type system and function call mechanism should be removed, the easiest way to customize is to write it in assembler.
Gabriel Ščerbák
Write it in LISP and you can return whatever you want...
Justin K
+31  A: 

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

Andreas Brinck
Well, it's possible for a function to return false on non-exceptional failure, while throwing only if something bad happens. But this is something you'd know from the start.
Steven Sudit
Thanks, but that wasn't my point. He wanted to leave it returning true even if the function were not throwing anything. And actually our local style states that we return false for errors and throw exceptions for apocalypse style failures. I guess some will claim it to be wrong, but that's for a different question :)
FireAphis
I'm confused then. If your local style states that you should return false for errors... isn't he doing the correct thing as dictated by style?
rossisdead
@rossisdead, the only problem is that this function doesn't have failure scenarios that require returning false. The only negative scenario is of log-the-error-and-exit kind that is usually handled by throwing an exception. Think of the exception here as a replacement for exit function.
FireAphis
Yeah, I would say that he correct according to your local style unless... it also says in what cases you must return void. Maybe you should add that to your local style
sims
Is signalling errors with a return value like `true`, `false` and `FileNotFound`?
Andrew Grimm
+26  A: 

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.

Bill the Lizard
+155  A: 
Jerry Coffin
+1 for YAGNI. Would mention KISS as well.
Tom
+1, developers are terrible at predicting the future.
Jesse Collins
@Tom: I hesitate to mention the KISS principle, simply because people often tend to misinterpret it. It's usually *well* worth making the function itself more complex if you can simplify its interface/usage by doing so. It definitely falls under the "as simple as possible, but no more so" category.
Jerry Coffin
@Jesse Collins: Definitely. If developers made their own weather forecast, you could recognize them without a problem. The guys with one sandal and one rubber boot, a thick jacket in combination with a bathing suit, a woolen hat, sunglasses, umbrella, skin covered in sun screen, and all sorts of things serving the purpose of being prepared for any possible future. I suppose, one half would freeze to death, and the other half through heatstrokes. And there'd be the lucky ones, getting away with just a cold.
back2dos
+10  A: 

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.

jpalecek
+1  A: 

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.

James Connell
There is no promise broken : The promise is "return true if successful, false if it failed". This is the interface. The fact this function will never fail is an implementation detail. This detail could even change from one version to another, or from an OS to another, etc..
paercebal
+22  A: 

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.

Pavel Shved
It might be possible that calling code unnecessarily checks for the return value. That would mean there's a lot of dead code that could be trimmed when changing to `void`.
Daniel Beck
@Daniel: and all of that code would probably be untested and all but ignored during coding and reviews – everyone knows it is unused anyway. Would you trust that code?
Christopher Creutzig
+3  A: 

+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 :)

Devil Jin
+2  A: 

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.

supercat
+4  A: 

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.

neal aise
If such a function doesn't throw exceptions and always returns true, would you write, as a user, the testing code ( if(ret==false) )? Will you also test how you handle the failure? And will your function propagate the failure or always succeed (assuming this is the only reason for a failure)?
FireAphis
@FireAphis: As I've mentioned, I wouldn't write such code :)
neal aise
Fun, I usually write `if (!ret)` rather than `ret == false`. Any reason to avoid the first one?
CFP
i wouldn't mind either one as long as it works. in fact, !ret would work for more languages than ret == false. so your choise is a touch good considering readability.
neal aise
+2  A: 

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.

Corey Ogburn
how to throw an unforseen error?
neal aise
Actually that's exactly our policy - return false on a fixable error, throw for catastrophic failures. In this case the function doesn't have any "fixable" errors. If it fails the system halts. What would you advise?
FireAphis
@neal aise, you can include a try/catch but only catch errors you can handle, other errors aren't caught and go up the call stack until something catches it, probably logs it, and the app shuts down (or enters the standby state I'm about to suggest). @FireAphis, I would advise that you find a gentle way to break the app, say if an unforeseen error happens you fall back to a standby phase, letting the user change some settings (proxy settings?) and offering them a way to retry.
Corey Ogburn
@FireAphis: An error should only be considered "fixable" if <i>existing</i> callers can be expected to do the right thing if it occurs. Any type of error to which the callers cannot be expected to react properly is should by definition be regarded as "not fixable". That doesn't leave a whole lot of room for possible fixable errors to arise, <i>unless</i> future callers might supply parameter values which present ones will not, and errors could arise when those values are supplied. In that case, the callers supplying such values may be expected to handle the new errors.
supercat
+49  A: 

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.

abelenky
Mock object are used to test this code, so technically I can make it run in "false" scenario.
FireAphis
You aren't testing the method if you are mocking it.
Lance Fisher
It's possible and reasonable to define a function, especially a virtual one, where a future implementation could return false but the present one would have no reason. In that case, a mock would be needed to test anything that could use the function, but there would be no need to test the function's ability to return false. For example, future versions of a function may support background I/O and return false for a "completed yet" status, while the present version might never return until complete. What's important is to test that <i>calling</i> code can handle the "not complete yet".
supercat
@supercat: What if you never end up creating that future implementation? Now you've forced all clients to do more work for no reason.
Ben
@Ben: There is another scenario where the Boolean might be reasonable: if the function accepts a mode parameter which could in future specify presently-unimplemented modes, some of which might have a reason to return false, the return need only be tested if/when such modes are used. My main point is: functions should never change so that existing callers receive behavior they do not expect. If the function will be allowed to return false in a way present callers will have to deal with, callers should be tested for it. Otherwise, the behavior with existing parameters must not change.
supercat
@supercat: "My main point is: functions should never change so that existing callers receive behavior they do not expect."Of course they should. You then update the callers to account for the new behavior. Your alternative is to attempt to support a lot of functionality that no one is currently using. This will make every caller's life difficult, and most likely YAGNI, anyway.
Ben
@Ben you can only change a non public function without pissing of your users. If you change a public function people will have to spend time and money fixing all the code you just broke. Its better to just write a new function and deprecate the old one.
josefx
@Lance: Stack Overflow is mocking it.
Andrew Grimm
+1  A: 

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.

Fabio Ceconello
+1  A: 

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!
}
Mark Ingram
+8  A: 

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.

NealB
+1  A: 

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

Collection (Java 2 Platform SE v1.4.2)

Shotgun Ninja
I'm not sure Java taking this strategy makes it advisable, especially in C++ in projects that don't use exceptions the same way Java does.
LnxPrgr3
Re-reading the Java example, they do have an arguably reasonable reason for doing this—a Collection may return false if an object is already present, and is thus not being added. Any other reason requires an exception.
LnxPrgr3
It may return false for collections which only store one instance of an Object such as Set, note that the there is no alternative ObjectAlreadyExistsException to be thrown
josefx
+1  A: 

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.

Larry Watanabe
+7  A: 

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.

Stefano Borini
Hmm, sounds reasonable indeed (+1). When you are telling this, I can understand this reasoning too. But he did the same in two other functions too that have nothing to do with initialization. I'll try to apply the same reasoning for those too, though all this design feels so wrong to me :(
FireAphis
@FireAphis: either the design is wrong, or the implementation is incomplete. To me, the design is wrong, but it's difficult to say for sure without looking at the actual code.
Stefano Borini
+27  A: 

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.

Mark Roddy
+1 good points. ` ` ` `
Pekka
This seems like a very correct description of the situation (+1). Actually the issue was closed with me saying that he can leave it as is if he states clearly in the documentation that this function currently returns only true. The problem is that I have some responsibility for the results of his work and I still feel bad for delivering what feels to me a flawed product.
FireAphis
@FireAphis: if he needs to write it in the documentation, that's still as wrong as it can be and requires everybody to code around it. See [my go on that remark later on](http://stackoverflow.com/questions/3505685/a-function-that-returns-only-true/3511121#3511121).
Abel
@FireAphis: Sadly, as long as you have coworkers there will be situations where you have to work around other people. As for a 'flowed product', all products get delivered with flaws. Being able to understand the cost/benefits of the fix and knowing when a flaw isn't worth fixing is all part of the job.
Mark Roddy
most enlightened answer I've read so far! This is a social problem, not a programming debate. I myself tend to get deadlocked because of style quibbles. I'm still not sure how to prevent and/or work my way out of these things, but one thing I do is when I'm reviewing code, and I see something I would have done differently, but see there are valid arguments to be made for what the other person has written, I make sure my review says something like "I don't insist that you make this change, but I personally like my way better". (continued...)
allyourcode
If I manage to convince them, that's great. If not, we don't waste time in an extended argument over something that probably won't be that important in the future.
allyourcode
+1  A: 

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.

John Sonmez
That's pretty much the worst of all worlds. The function signature not only allows you to silently ignore errors, but requires it.
Dennis Zickefoose
A: 

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

Avram
+2  A: 

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.

Dinah
+1  A: 

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

Sarath
A: 

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.

Gan
+1  A: 

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.

Carlos
I personally think that this forward compatibility will never be required. It is highly improbable that any such change will be ever made. But that's my personal opinion, of course. My opponent disagrees with me, although neither he can foretell when this change will be needed.
FireAphis
+19  A: 

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.

Abel
Fleshing out of the method contract also illustrates another shortcoming: How is the caller supposed to act upon receiving false if the contract doesn't define what false means? And if the contract defines what false means, it not likely to be future proof, is it?
meriton
@meriton: very valid points indeed!
Abel
I wish I could up-vote this more than once!
Wesley Wiser
+3  A: 

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.
paercebal
+2  A: 

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.

Max
+1 for paying attention also to the review process. It is more important to have a method to solve these disagreements, than to find the correct answer to this particular disagreement - which is, of course, that of the poster.
Daniel Daranas
+1  A: 

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.

back2dos
A: 

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.

subanki
A: 

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.

Android Eve
+1  A: 

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

pm100
A: 

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).

ChrisW
+1  A: 

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.

doc
A: 

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 :)

ama
A: 

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.

Svante
+1  A: 

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.

+1  A: 

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.

Alf P. Steinbach