Using return value vs out parameters

Question

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?

Answer 1

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.

Answer 2

I would go with

bool TryDoSomething([some arguments]);

, following the naming logic of Int.TryParse.

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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();

Answer 7

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

Answer 8

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.

Answer 9

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();

Answer 10

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.

Answer 11

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;

Answer 12

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.

Answer 13

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.

Answer 14

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.