Is using "out" bad practice

Question

I have just added an out bool parameter to a method I've written in order to get a warning in to my UI. I've used an out rather than getting the method itself to return false/true as that would imply the DoSomething failed/succeeded. My thinking was that the warnUser would indicate what the warning actually was without having to look at the implementation of the method.

Original Code

public void DoSomething(int id, string input);

New Code

public void DoSomething(int id, string input, out bool warnUser);

I'm using Moq to test this code, but it doesn't support out/ref parameters because they're not supported by Lambda expressions

Test Code

mockService.Verify(It.IsAny<int>(), It.IsAny<string>(), It.IsAny<bool>());

So, is using out parameters bad practise and if so what do I do instead?

Answer 1

Using an out parameter in a void method is generally a bad idea. You say you've used it "rather than getting the method itself to return false/true as that would imply the DoSomething failed/succeeded" - I don't believe that implication is there. Usually in .NET failure is indicated via an exception rather than true/false.

out parameters are generally uglier to use than return values - in particular, you have to have a variable of the right type to handle, so you can't just write:

if (DoSomething(...))
{
   // Warn user here
}

One alternative you might want to consider is an enum indicating the warning level required. For example:

public enum WarningLevel
{
    NoWarningRequired,
    WarnUser
}

Then the method could return a WarningLevel instead of bool. That would make it clearer what you meant - although you might want to rename things slightly. (It's hard to give advice with metasyntactic names such as "DoSomething" although I entirely understand why you've used that here.)

Of course, another alternative is that you might want more information to be present - like the reason for the warning. That could be done with an enum, or you might want to give some richer result entirely.

Answer 2

out is very much a useful construct, in particular in patterns like bool TryGetFoo(..., out value), where you want to know the "if" and the "what" separately (and nullable isn't necessarily an option).

However - in this case, why not just make it:

public bool DoSomething(int id, string input);

and use the return value to signal this?

Answer 3

A few additional thoughts:

• What FxCop says: CA1021: Avoid out parameters

• For private/internal methods (implementation details) out/ref parameters are less of an issue

• C# 7 Tuples often are a better alternative to out parameters

• Further, C# 7 improves the handling of out parameter on the call site by introducing "out variables" and by allowing to "discard" out parameters

Answer 4

Given a big Legacy Code Method (say 15+ lines, legacy code methods with 200+ lines are quite common), one might want to use the "Extract Method" refactoring to clean up and make code less brittle and more maintainable.

Such a refactoring is very likely to produce one or more new methods with out parameters for setting variable values local to the prior method.

Personally, I use this circumstance for a strong indicator that there have been one or more descrete classes hiding in the prior method, so that I can use "Extract class", where in the new class those out parameters get properties visible to the calling (prior) method.

I consider it bad practice to leave the extracted methods with out parameters in the prior method, skipping the coherent following step of "Extract Class", because if you skip it, those local variables remain in your prior function, but do semantically no longer belong to it.

So, in such a scenario, out parameters in my opinion are a code smell breaking SRP.

Answer 5

This is a really tough question to answer without knowing more about the context.

If DoSomething is a method in the UI layer that is do something which is UI related, then perhaps it's okay. If DoSomething is a business layer method though, then this is probably not a good approach because it implies that the business layer needs to understand what an appropriate response might be, and it might even have to be aware if localization issues.

On a purely subjective note, I've tended to stay away from out parameters. I think they disrupt the flow of the code and make it a little less clear.

Answer 6

I think that the best way to do this is to use expected Exceptions. You can create a set of custom exceptions that define your warnings, and catch them in the calling method.

public class MyWarningException : Exception() {}

...

try
{ 
     obj.DoSomething(id,input);
}
catch (MyWarningException ex)
{
     // do stuff
}

The idea would be that the exception is raised at the end of the DoSomething implementation, so that the flow doesn't stop in the middle

Answer 7

I think the best best solution for your case is to use a delegate instead of bool. Use something like this:

public void DoSomething(int id, string input, Action warnUser);

You can pass an empty lamda where you don't want to display warnings to user.