36

If there is a method

bool DoStuff() {
    try {
        // doing stuff...
        return true;
    }
    catch (SomeSpecificException ex) {
        return false;
    }
}

should it rather be called IsStuffDone()?

Both names could be misinterpreted by the user: If the name is DoStuff() why does it return a boolean? If the name is IsStuffDone() it is not clear whether the method performs a task or only checks its result.

Is there a convention for this case? Or an alternative approach, as this one is considered flawed? For example in languages that have output parameters, like C#, a boolean status variable could be passed to the method as one and the method's return type would be void.

EDIT: In my particular problem exception handling cannot be directly delegated to the caller, because the method is a part of an interface implementation. Therefore, the caller can't be charged with dealing with all the exceptions of different implementations. It is not familiar with those exceptions. However, the caller can deal with a custom exception like StuffHasNotBeenDoneForSomeReasonException as was suggested in npinti's answer and comment.

Limbo Exile
  • 705
  • 2
  • 7
  • 11
  • 2
    It depends on the usage of the function and the stuff that is being done. If it is necessary, that the stuff needs to be executed correctly, then I would consider this approach flawed, because the user of the function might miss the boolean flag and also lacks information provided by the exception. – Benni Jun 11 '14 at 09:49
  • 7
    Most of the time, I'd call it "broken" as returning a `boolean` instead of wrapping or passing the exception is nearly always wrong. – maaartinus Jun 11 '14 at 14:54
  • @maaartinus And how would you handle a situation where the exception only has meaning within the method itself, and externally you only care whether or not the procedure succeeded but not about the specifics (assuming the wrapped exceptions are specific to the procedure and not generalized)? For example, with Java reflection, there's no method that checks for the existence of a method on a class, only methods to retrieve a `Method` object or throw an exception when the requested method does not exist (you could retrieve the list of methods for the class and search that, but it's not one call). – JAB Jun 11 '14 at 15:16
  • (The main usage for that would be interfacing with some other language that isn't as strictly typed as Java and relies heavily on, say, passing messages as strings for one reason or another.) – JAB Jun 11 '14 at 15:18
  • 2
    This kind of exception handler is rarely a good idea. Only catch specific exceptions you're expecting, not all of them. And if possible avoid throwing it in the first place, eliminating the need to catch it. – CodesInChaos Jun 11 '14 at 17:12
  • 2
    Umm...`BadlyDesignedMethodInSeriousNeedOfRefactoring`? And to answer your question about the exceptions - I'd either let the caller handle them, or catch them and then throw a custom exception that means "this method no do its job". Share and enjoy. – Bob Jarvis - Слава Україні Jun 11 '14 at 22:54
  • As indicated by the large number of comments saying this is a Bad Idea™, it would probably be helpful if you gave some more details about *why* you need to do this. In general, this is rarely a good design. – jpmc26 Jun 12 '14 at 00:45
  • 5
    To all those who are saying: just throw (or let pass) an exception, you are making unfounded assumptions on how this code is being used. One possible scenario is that there is a problem to be solved, with various heuristic solution methods that solve ever bigger subclasses of the problem at ever increasing cost; it would make sense to write something like `if (FirstMethodSucceeds(problem) or SecondMethodSucceeds(problem) or ...) Hurray(); else UniversalSolve(problem);`. Doing the same with (custom?) exceptions would be uselessly more complicated. – Marc van Leeuwen Jun 12 '14 at 12:01
  • The answers, and discussion here, are in the realm of "use exceptions or not." Can you _assume_ a call was successful if it doesn't throw an exception? If so, you'd never see a false result (in the specific example here, but just a getter or checker). Yet, this pattern is used everywhere. Database calls, network calls, file system calls, etc. often return a success or failed value. Put another way, false does not have to be an exception. In these scenarios, true and false could be equally weighted, neither being more exceptional than the other. – lilbyrdie Jun 12 '14 at 14:25
  • Related question: https://softwareengineering.stackexchange.com/questions/161753/naming-a-do-x-if-needed-method – imgx64 Apr 24 '18 at 04:48

9 Answers9

69

In .NET, you often have pairs of methods where one of them might throw an exception (DoStuff), and the other returns a Boolean status and, on successful execution, the actual result via an out parameter (TryDoStuff).

(Microsoft calls this the "Try-Parse Pattern", since perhaps the most prominent example for it are the TryParse methods of various primitive types.)

If the Try prefix is uncommon in your language, then you probably shouldn't use it.

stakx
  • 2,138
  • 18
  • 29
ne2dmar
  • 624
  • 5
  • 8
  • 3
    +1 This is how I have seen this sort of thing done elsewhere. `if (TryDoStuff()) print("Did stuff"); else print("Could not do stuff");` is a pretty standard and intuitive idiom in my opinion. – Karl Nicoll Jun 11 '14 at 14:56
  • 12
    It should be noted that `TryDoStuff` methods are assumed to fail cleanly and to have no side effect when they return false. – Trillian Jun 11 '14 at 22:24
  • FYI, there is also `Remove` methods for example in the .NET framework that will remove an element from a data structure (e.g. `Dictionary`), and return a `bool`. The consumer of the API can decide to consume it or ignore it. However, errors are reported as exceptions. – Omer Iqbal Jun 25 '14 at 04:23
18

What if you simply threw the exception to the calling code?

This way you are delegating the exception handling to who ever is using your code. What if, in the future you would like to do the following:

  • If no exceptions are thrown, take action A
  • If (for instance) a FileNotFoundException is thrown, take action B
  • If any other exception is thrown, take action C

If you throw back your exception, the above change would simply entail the addition of an extra catch block. If you leave it as is, you would need to change the method and the place from which the method is called, which, depending on the complexity of the project, can be in multiple locations.

npinti
  • 1,654
  • 10
  • 12
  • 1
    What if the exception is of kind that can't be delegated and should be dealt with internally? – Limbo Exile Jun 11 '14 at 09:52
  • 2
    @LimboExile: I think that you could still deal with it internally and then, maybe throw another exception, maybe your own. This would illustrate that something that shouldn't have happened took place, but at the same time, you did not expose what is really going under the hood, which I assume is why you want to deal with the exception internally. – npinti Jun 11 '14 at 09:57
  • @LimboExile how does the inside of the function know that? the caller should try/catch it then. – djechlin Jun 11 '14 at 14:51
  • More to the point, apparently it can't be dealt with internally, because the caller still needs to know about. You can wrap in a simpler exception though, like `CouldNotDoStuffException`. – djechlin Jun 11 '14 at 14:54
  • 6
    It's perhaps worth bearing in mind that throwing an exception should be for an *exceptional case*. If it is common or normal for `DoStuff` to fail, throwing an exception for the failure case would be akin to controlling program flow with exceptions which is bad. Exceptions also have an inherent performance cost. If `DoStuff` failing is an uncommon case due to an error, then exceptions definitely are the way to go as @npinti suggests. – Karl Nicoll Jun 11 '14 at 14:54
  • 1
    @KarlNicoll Whether something is "exceptional" is completely subjective. The main criteria should be whether you want the program to crash if no one does something about the error, and whether you want the possibility of error to be present in the function's type. Additionally, it's not a fact that exceptions are expensive; it depends on the language and how you throw them. Exceptions are cheap in Python, and if you strip the stack trace information they can be cheap in Java as well. Even if there was a performance cost, it's premature optimization to worry about it until you've profiled. – Doval Jun 11 '14 at 15:05
  • 1
    @Doval - I agree that it's subjective, which is why OP shouldn't discount npinti's answer entirely, as it could well be the best course of action. My point was that throwing exceptions isn't always the best path to take. There are plenty of cases where a failure doesn't justify throwing an exception and potentially crashing an application. For example, if OP's `DoStuff()` method cleans up after the inner code throws an exception, and the Post-conditions of the method are still correct, a return code may be a better option as the error has been handled. – Karl Nicoll Jun 11 '14 at 15:52
  • 1
    @Doval about premature optimization: exceptions are often fundamental architectural aspect, and by the time there is meaningful profiling data, it's too late to change, it'd need too many changes. Premature optimization argument only really applies to internal implementations, parts that can be easily changed. – hyde Jun 12 '14 at 06:54
  • @hyde I'm not convinced about the "too many changes" part. The biggest performance gains come from optimizing a relatively small amount of code. You could make the same argument for garbage collection too, since optimizing memory usage often involves architectural changes (e.g. memory pools) but no one in their right mind starts with those tricks. – Doval Jun 12 '14 at 11:17
  • I think the worst part about exceptions is that you can't know as a client that you need to catch them unless you either read documentation, source code or just magically "know". While I'm not thrilled about bool flags as return values, I still prefer this being represented in the type system. This is where algebraic data types and/or tuples + pattern matching really shine, and OOP languages have a lot to learn. – sara Jun 16 '16 at 18:27
8

In Java, the Collection API defines an add method that returns a boolean. It basically returns whether the add changed the collection. So for a List it will usually return true, since the item was added, while for a Set it might return false, if the item was already there, since Sets allow each unique item at most once.

That said, if you add an item that is not allowed by the collection, for example, a null value, the add will throw a NullPointerException rather than returning false.

When you apply the same logic to your case, why do you need to return a boolean? If it is to hide an exception, don't. Just throw the exception. That way you can see what went wrong. If you do need a boolean, because it might not have been done, for some other reason than an exception, name the method DoStuff(), since that represents the behavior. It does stuff.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
mrjink
  • 718
  • 5
  • 12
8

DoStuff() is enough, and the returning value of the function should be documented, and you don't need to mention it in the function name, looking for many of API available:

PHP

// this method update and return the number affected rows 
// called update instead of updateAndGetAffectedCount
$affected = $query->update(/* values */);

C-Sharp

// Remove item from the List
// returns true if item is successfully removed; otherwise, false.
public bool Remove( T item )
amd
  • 181
  • 3
1

Might fail for different reasons, Exception, normal conditions, so would use this:

boolean doStuff() - returns true when successful

Important is to document what the boolean means.

user136354
  • 19
  • 1
1

I usually have methods return an OperationResult (or OperationResult<T>) and set the IsSuccess property on the OperationResult appropriately. If the 'usual' method is void then return OperationResult, if the 'usual' method returns an object then return OperationResult<T> and set the Item property on the OperationResult appropriately. I'd also suggest having methods on OperationResult such as .Failed(string reason) and .Succeeded(T item). OperationResult quickly becomes a familiar type in your systems and developers get to know how to handle it.

0

It really depends on the language that you're using. Like for some languages there are conventions and protocols that really don't exist in many languages or any at all.

For example, in the Objective-C language and the iOS/Mac OS X SDK method names usually go along the lines of:

- (returndatatype) viewDidLoad {
- (returndatatype) isVisible {
- (returndatatype) appendMessage:datatype variablename with:datatype variablename {

As you can see, there's a method there called viewDidLoad. I prefer to use this type of naming whenever I make my own applications. This could be used to check if something was supposed to happen as you said. I believe that you're thinking too deeply about what it is you name your methods. Most methods return the status of what it is they do regardless, take example:

In PHP, mysql_connect() will return false if the connection fails. It doesn't say it will, but it does and the documentation says it does so it can't be misinterpreted to the programmer.

Gergy008
  • 1
  • 2
  • Ah, but viewDidLoad is more of a callback notification. Users don't call it to load the view. Rather, it is called after the view is loaded. Likewise: isVisible doesn't set the visibility, rather it just returns what the current value. It doesn't _do_ anything. – lilbyrdie Jun 12 '14 at 14:14
0

I would like to suggest to use the 'Try' prefix. This is a well known pattern for situations that the method may succeed or not. This naming pattern has been used by Microsoft in .NET Framework (for example, TryParseInt).

But, maybe this is not about naming. It is about how you design your method's input/output parameters.

My idea is that if a method has a return value, then the purpose of calling that method should be to get that return value. So, you should avoid using return type to indicate the status of an operation.

In C#, I prefer to use an out parameter. Using an out I am marking the parameter as a side parameter. Specially that C# 6 will support inline variable declaration.

DoStuff(out var isStuffDone); // The out parameter name clearly states its purpose.
DoStuff(out var _); // I use _ for naming parameters that I am ignoring.
Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
000
  • 4,525
  • 4
  • 16
  • 16
0

I would pick a name based on the primary role of the method. The fact, that method returns boolean value doesn't mandate 'Is' prefix. Let's have some Java code, which uses 'Is' prefix quite extensivelly. Take look at java.io.File.delete(). It returns boolean, but it isn't called isFileDeleted(). The reason is that the primary role of the metod is to delete a file. Someone calls this method with the intention to delete a file.

The boolean is there just to comunicate back the result of the work. On the other hand let's see java.util.Map.isEmpty(). Obviously, you call such a method to find out if the collection is empty. You do not want any stuff done. You just want to find out what the current state is.

So personally, I would definitelly stick with DoStuff().

This is somewhat a very important issue for me. Many times when I maintain legacy code, I do stumble upon something I call personally "lying method". Name suggests action A, but the body does action B. The most bizzare example beeing a getter method changing data in the database.

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
Jacek Prucia
  • 2,264
  • 1
  • 17
  • 15