Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

It's very popular to see code like this:

bool DoSomething([some arguments]);

You can't understand the result of this method by looking at its name (it could actually be a void).

What's the best approach to refactor such kind of methods? What I have in mind is something like this:

void DoSomething([some argumens],out bool everythingIsOk);

What do you think?

EDIT

It seems that so many people prefer

bool TryDoSomething();

rather than

void DoSomething(out bool success);

But what if a method is going to have a result instead of being void? Can we use int.TryParse pattern or we should return the result and throw an exception if something goes wrong?

share
    
Use exceptions unless you need performance and this private method is just a helper. –  Leonid Feb 23 '12 at 19:04
comments disabled on deleted / locked posts

locked by Jamal Dec 6 '13 at 18:17

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

14 Answers

You’re asking the wrong question. In reality, both methods are bad. If you need the success message, either throw an exception on failure (but only if this is really an exceptional scenario, or return the success and indicate this in the method name. For instance, by calling the method TrySomething.

In general, your example sounds like the method is actually doing two distinct things, otherwise there would be no confusion in the first place. But this is code smell anyway: every method should have only one purpose. If that is the case, then the meaning of the return value is always clear, no need to give it a name.

share
add comment

I would go with

bool TryDoSomething([some arguments]);

, following the naming logic of Int.TryParse.

share
2  
Please don't use this. Exceptions are meant for this. If you're worrying about performance because of Exception creation check the preconditions before calling this method. –  dvdvorle Feb 22 '12 at 23:11
28  
@MrHappy: Exceptions absolutely are not meant for this. Exceptions are meant for exceptional conditions, meaning they are conditions that should not normally occur while your program is executing. Your code should always preemptively check to see if the data is valid. Exceptions are not meant to serve as lazy validation routines. –  Adam Robinson Feb 23 '12 at 3:42
2  
@AdamRobinson: Tell Microsoft that. int.Parse and int.TryParse are prime examples. TryParse is only here because of performance reasons. Should I check that the string I want to parse only contains characters that can be converted to numbers? No I expect my input to be valid. If it's not valid, that is an exceptional condition, and a FormatException will be thrown by int.Parse which I can handle in a seperate catch clause. I can't miss an exceptions, whereas I can miss a boolean return value. –  dvdvorle Feb 23 '12 at 9:07
14  
@MrHappy Nonsense, TryParse is not there for performance reasons. Rather, it was retrofitted once they saw that throwing an exception when parsing is highly inappropriate. Eric Lippert acknowledges that int.Parse was an “unfortunate design decision”. –  Konrad Rudolph Feb 23 '12 at 10:11
3  
@MrHappy But this isn’t what exceptions are good at. They are good at indicating exceptional conditions that don’t necessarily have to be handled by the caller, but instead passed on to higher levels. This is never true when parsing input. Faulty input should always be handled straightaway. Think of it this way: if you need to always put a try block around a call, then exceptions aren’t appropriate here. They are only appropriate when the caller can usually skip the try and let exceptions bubble up. Read Eric Lippert’s explanation that I linked above. –  Konrad Rudolph Feb 23 '12 at 12:41
show 22 more comments

The first form is indeed misleading - you have no reliable way of knowing what that bool value means.

The second is better, as the parameter name (everythingIsOk) is descriptive enough.

In cases like these, however, I'd use this signature:

void DoSomething([some arguments]);

and throw an appropriate Exception if anything goes wrong.

I see two advantages to this approach:

  • it doesn't mix inputs and outputs - I think it's nice that the list of arguments only contains those needed for the method to do its job
  • in case something is not OK you can give extensive details about what went wrong (by throwing an appropriate type of Exception and/or by populating the exception's Message)

There's one scenario where I'd avoid this, though: if your method is called many times, the creation of the `Exception object may be expensive, resulting in a performance penalty. In most cases, however, this will not be a problem.

Update:

There's another possibility - returning a result object that has a bool property indicating success state. This is used for instance by .NET's Regex.Match(...) method (it returns a Match object that has a bool Success property).

Our method signature would be:

SomeResult DoSomething([some arguments]);

The caller could inspect the value of SomeResult.IsSuccesful to see if all went OK.

share
    
...Except if this is being used in a multi-threaded context, re-using an exception would get really ugly. I'd rather have guaranteed safety over (somewhat) smaller costs. –  Clockwork-Muse Feb 22 '12 at 20:31
5  
If your code is throwing exceptions repeatedly, your code is broken. Full stop. Exceptions are meant for exceptional conditions, not data validation. –  Adam Robinson Feb 23 '12 at 3:43
1  
@AdamRobinson: That's true in c#, but not in e.g. Python. –  Neil G Feb 23 '12 at 7:29
1  
@AdamRobinson - that's not a general rule. After all, .NET v4.0 has a class called ValidationException. A Google search tells me that Java has one too. –  w0lf Feb 23 '12 at 7:29
    
@w0lf: That's the exception, though (pardon the pun). Validation exceptions like that are intended for use in places where there's nothing that the program can do about the fact that the data's invalid and the fact that it's invalid is an exceptional condition (in other words, there should be actual, bona-fide data validation at a higher level that somehow mistakenly passed this invalid value down). –  Adam Robinson Feb 23 '12 at 13:01
show 1 more comment

Not a good idea IMO. Too much extra work and potential gotchas for something that is easily solved with good code comments.

Using out has implications if overloading such a function. The compiler cannot differentiate between ref and out.

Cannot pass properties as out parameters.

The calling code must declare and assign an "extra" variable associated with the out parameter.

share
2  
+1 for great critics :) but what's the best solution in your opinion ? since method comments can't be seen when you are reviewing code but a variable name can clarify the purpose though we should add an extra line to our code ? –  Beatles1692 Feb 22 '12 at 17:56
1  
I'm assuming Visual Studio is being used here. Use the "///" to make your comments. stackoverflow.com/questions/429452/… –  radarbob Feb 25 '12 at 5:04
add comment

I think the main problem here is not the code itself, its that there's no xml comments. Seriously, commenting can go a long way. I'd address that first. Second, I'd look at the name of the method itself. Is it doing a check? Then CanDoSomething, IsSomething, etc would work. If the method actually does "do something" and it's returning success or failure, then the name is fine. Another alternative though might be an exception, depending on if the call should normally work or if failure is pretty common.

Regarding your suggested change with an out parameter, I couldn't do that. Its not more expressive, and it forces callers to create a variable to hold the result. In general, I think you should steer away from out / ref paramaters, they should be used pretty rarely.

share
add comment

I think the key question here is whether the boolean being false indicates an 'exceptional' problem or not. If it does, then forget the boolean and throw an exception.

Otherwise, you should separate out the validation from the execution:

if (canDoSomething([some arguments]) {
   doSomething([some arguments]);
} else {
   doSomethingElse([some arguments]);
}

and one might go to a factory method from there:

OperationToDoSomething operation = getAppropriateOperation([some arguments]);
operation.doIt();
share
    
...which will fail horribly the instant DoSomething() depends on any external resource with a non-zero probability of intermittent availability. –  Michael Kjörling Feb 23 '12 at 8:24
    
...which wasn't part of the question, and presumably would have been. I would usually think of intermittently unavailable as an exceptional case though, at least at this level of abstraction. –  sharakan Feb 23 '12 at 14:05
1  
I agree with @sharakan in this context. If you look at the msdn.microsoft.com/en-us/library/system.io.stream.seek.aspx Streams class, you can see how they prefer you to use the CanSeek property before you use the Seek method. This is very appropriate in many cases, but, as Michael points out, it could be a problem in a concurrency situation. –  Pat Feb 24 '12 at 17:44
add comment

How about naming the method CanDoSomething? Prefixing the method name Can, Is, Has, or Should implies a Boolean result.

share
2  
It also implies that the method has no side effects though. So maybe you should implement that separately. –  dvdvorle Feb 22 '12 at 23:14
add comment

Whenever I find out I need more than one output from a method, this means:

  1. My method does more work than it should, so I'll try to refactor
  2. The data returned deserves its own struct/class, even an private one, with 2 or more properties for the real result of the operation and the flag for success, like:

Code:

private struct ComplexOperationResult
{
   public bool IsOperationSuccessful{get;set;}
   public MyRealData RealData{get;set;}    
}

The notations like Int32.TryParse are useful, but only for some infrastructure/base operations for a framework. Previous implementations (I'm not sure for the current one) of Int32.TryParse were only wrapper around try{int32.parse}catch{..}, because this is such a common operation, and to avoid a lot of boilerplate code.

So, if it's a public framework/library method, which will be used a lot, and exposing it otherwise will lead to too much additional repetitive coding for the consumers, go for TryXYZ pattern. Otherwise, introduce proper result structure, or refactor/rethink the method.

share
add comment

Thinking in terms of what the caller will look like, I feel the first one is better.

if(DoSomething())
{
    DoSomething1();
}
else
{
    DoSomething2();
}

vs.

bool b=false;
DoSomething(b);
if (b)
{
    DoSomething1();
}
else
{
    DoSomething2();
}

or even consider if you are calling this function in return statement, it could simply be called as

return DoSomething();
share
    
I'd rather rename b to success :) besides you can write the first one like this if(DoSomething()) instead of comparing it with true –  Beatles1692 Feb 22 '12 at 17:52
    
Good suggestions Beatles. But my companies coding guideline do not allow the second . :-) –  Manoj R Feb 22 '12 at 18:05
2  
why they don't allow ? if I may ask –  Beatles1692 Feb 22 '12 at 18:10
add comment

Unless DoSomething is a trivial operation I would return a custom Result object that holds more information about what has happened. For instance:

public class Result
{
    public bool WasSuccessful { get; private set; }
    public string Message { get; private set; }
}

var result = DoSomething();

if (result.WasSuccessful)
{
   // 
}
else
{
    Console.Write(result.Message);
}

This way you can communicate back a lot more information than just a true/false value and make the properties have more meaningful values. The values I supplied are just illustrative, and obviously will change based on your requirements (as should the name of the "Result" object). It also means you can easily add extra properties/information later without breaking the method signature.

share
add comment

I would like to use RETURN VALUE in your case. Sometime it is really easy to use. like below ↓

.Append(MethodA()).Append(MethodB())......

I only use the OUT VALUE to save the status in each step(method) of a entire process , not for the result value.

And in C#4.0 you also can Cast the child class to parent by OUT keyword.

public delegate T Find<out T>();
Find<Cat> cat = ()=>new Cat();
Find<Animal> animal = cat;
share
add comment

A slightly different perspective: it's always easier to go asynchronous with the void version. I admit I'm biased by my own experiences which narrow on the parallel, non-deterministic environments, but in that context it's easier to think in autonomous independent async invocations. I don't say that my way of thinking is any better than that of anyone else though, consider it rather an opinion.

share
    
Could you expand on that? How does using void make it easier to go asynchronous? –  svick Feb 1 '13 at 15:56
    
When using void, the last parameter is typically the out param. When going async, it's easier to replace that with a callback reference. –  András Hummer Feb 5 '13 at 8:45
add comment

In general, a method's return value as in bool DoSomething() is the result that is deliberately intended by the programming logic. Put in other words, the return value of a method must represent the purpose of calling that method.

On the other hand, an output out parameter modifier (there is also a ref modifier in C#), implies an additional information that is generated during the method execution. This information is not the goal of the method, but still it is necessary to be supplied, because it may be used latter, or because it could be tedious to obtain that information a second time.

For instance, take the bool int.TryParse(string value) method. It implies that you are interested in checking if value is an integer, regardless of the actual integer it represents. In other words, you you ask if some string is a number and expect the computer to say: "yes, it is a n integer" or "no, it is not". The out parameter in this case represents the parsed integer, that is produced only if the parsing was successful.

Consider for instance the following code:

if (int.IsValidInt(value)) // an imaginary method
{
    int theInt = int.Parse(value);
}

Assume that the int.IsValidInt(value) method (that does not originally exist in .NET) checks if a string is integer by internally parsing it. It will produce an integer corresponding to the value parameter, but we will not be able to use it latter. So, if we need it, we will have to parse it again (via the int.Parsemethod), thus repeating the same operations already done by the int.IsValidInt alongside all the associated CPU and memory usage.

In such cases (like the one determining if a string represents an integer) we may have to produce a secondary result, that should be used latter. This is a good enough reason to use an an output parameter, as done in the int.TryParse method. In regards to choosing which to be the return value, I think it should be the one that best represents the intentions of the method, and any additional data should be supplied as an output parameter.

According to the int.Parse vs. int.TryParse question, it depends on your case. If you need to to execute different logic depending on the input being a number or not, the int.TryParse seems more natural to use as it implies the logical branching:

if (int.TryParse(input, out parsed))
{
    int importantCalculation = parsed*someMagicNumber;
    /// other business logic
}
else
{
    // ask for input again
}

In case you need to operate with the input, you have a choice:

  1. Use int.Parse(input); directly. This will cause an exception if the input is not valid, and you may have to handle the exception using try-catch blocks. Do this, if you need to show the user information for the error, or if invalid integers are unacceptable in this case.

  2. Use the int.TryParse in combination with if-else. This can be useful if you need faster code (exception slow down the runtime), or if you have an alternative logic flow that can be executed.

share
add comment

In this case is not requied to refactor but what if you need two output parameters? Example:

public bool DoSomething(object parameter, out ref string ErrorDescription)
{
try {
...
}
catch (Exception ex)
{
 ErrorDescription = ex.Message;
 return false;
}
ErrorDescription = "ok.";
return true;
}

If you would not use output parameter, you had to use any public property or create class as return type. This is easier.

share
add comment

protected by Jamal Dec 6 '13 at 9:51

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.