Split the calculation of return value and the return statement in one-line methods?

Question

I have had a discussion with a coworker about breaking a return statement and the statement that calculates the return value in two lines.

For example

private string GetFormattedValue()
{
    var formattedString = format != null ? string.Format(format, value) : value.ToString();
    return formattedString;
}

instead of

private string GetFormattedValue()
{
    return format != null ? string.Format(format, value) : value.ToString();
}

Code-wise I don't really see a value in the first variant. For me, the latter is clearer, particularly for methods that short. His argument whatsoever was that the former variant is easier to debug - which is quite a small merit, since VisualStudio allows us a very detailed inspection of the statements, when the execution is stopped due to a break point.

My question is, if it's still a valid point to write code less clear, just to make debugging a glimpse easier? Are there any further arguments for the variant with the split calculation and return statement?

Answer 1

Introducing explaining variables is a well-known refactoring which can sometimes help to make complicated expressions better readable. However, in the shown case,

• the additional variable does not "explain" anything which is not clear from the surrounding method name
• the statement gets even longer, so (slightly) less readable

Moreover, newer versions of the Visual Studio debugger can show the return value of a function in most cases without introducing a superfluous variable (but beware, there are some caveats, have a look at this older SO post and the different answers).

So in this specific case, I agree to you, however, there are other cases where an explaining variable can indeed improve code quality.

Answer 2

Given the facts that:

a) There is no impact on the final code as the compiler optimises the variable away.

b) Having it separate enhances debugging capability.

I've personally come to the conclusion that's it's a good practice to separate them 99% of the time.

There are no material disadvantages to doing it this way. The argument that it bloats code is a misnomer, because bloated code is a trivial issue in comparison to unreadable or hard-to-debug code. Furthermore, this method cannot by itself create confusing code, that's entirely up to the developer.

Answer 3

Often, introducing a variable just to name some result is very helpful when it makes the code more self documenting. In this case that's not a factor because the variable name is very similar to the method name.

Note that one line methods don't have any inherent value. If a change introduces more lines but makes the code clearer, that's a good change.

But in general, these decisions are highly dependent on your personal preferences. E.g. I find both solutions confusing because the conditional operator is being used unnecessarily. I'd have preferred an if-statement. But in your team you may have agreed on different conventions. Then do whatever your conventions suggest. If the conventions are silent on a case like this, notice that this is an extremely minor change that doesn't matter in the long run. If this pattern occurs repeatedly, maybe initiate a discussion how you as a team want to handle these cases. But that is splitting hairs between “good code” and “perhaps a tiny bit better code”.

Answer 4

In response to your questions:

My question is, if it's still a valid point to write code less clear, just to make debugging a glimpse easier?

Yes. In fact, part of your earlier statement seems to me (no offense) to be a little short-sighted (see bold below) "His argument whatsoever was that the former variant is easier to debug - which is quite a small merit, since VisualStudio allows us a very detailed inspection of the statements, when the execution is stopped due to a break point."

Making debugging easier is (almost) never of "small merit" because by some estimates 50% of a programmer's time is spent debugging (Reversible Debugging Software).

Are there any further arguments for the variant with the split calculation and return statement?

Yes. Some developers would argue that split calculation is easier to read. This, of course, helps with debugging but also aids when someone is trying to decipher any business rules that your code may perform or apply.

NOTE: Business rules may be better served in a database since they can change often. Nevertheless, clear coding in this area is still paramount. (How to Build a Business Rules Engine)

Answer 5

I'd go even further:

private string GetFormattedValue()
{
    if (format != null) {
        formattedString = string.Format(format, value);
    } else {
        formattedString = value.ToString()
    }
    return formattedString;
}

Why?

Using ternary operators for more complex logic would be unreadable, so you'd use a style like the above for more complex statements. By always using this style, your code is consistent and easier for a human to parse. Additionally, by introducing this kind of consistency (and using code lints and other tests) you can avoid goto fail type errors.

Another advantage is that your code coverage report will let you know if you forgot to include a test for format is not null. This wouldn't be the case for the ternary operator.

---

My preferred alternative - if you're in the "get a return as quick as possible crowd" and not against multiple returns from a method:

private string GetFormattedValue()
{
    if (format != null) {
        return string.Format(format, value);
    }

    return value.ToString();
}

So, you can look at the last return to see what the default is.

It's important to be consistent though - and have all your methods follow one or the other convention.

Answer 6

I don't think that such technique can be justified by the need to debug. I've encountered this approach myself a thousand times, and from time to time I keep doing this, but I always keep in mind what Martin Fowler said about debugging:

People also underestimate the time they spend debugging. They underestimate how much time they can spend chasing a long bug. With testing, I know straight away when I added a bug. That lets me fix the bug immediately, before it can crawl off and hide. There are few things more frustrating or time wasting than debugging. Wouldn't it be a hell of a lot quicker if we just didn't create the bugs in the first place?

Answer 7

I think some people are getting hung up on issues tangential to the question, such as the ternary operator. Yes, lots of people hate it, so maybe it's good to bring up anyway.

Concerning the focus of your question, moving the returned statement out to be referenced by a variable...

This question makes 2 assumptions that I disagree with:

1. That the second variant is more clear or easy to read (I say the opposite is true), and

2. that everyone uses Visual Studio. I have used Visual Studio many times and can use it just fine, but I usually am using something else. A dev environment that forces a specific IDE is one I would be skeptical of.

Breaking something out to a named variable rarely ever makes anything harder to read, it almost always does the opposite. The specific manner in which someone does it could cause problems, like if a self-documentation overlord does var thisVariableIsTheFormattedResultAndWillBeTheReturnValue = ... then obviously that is bad, but that is a separate issue. var formattedText = ... is fine.

In this specific case, and possibly many cases since we are talking about 1-liners, the variable would not tell you much that the function name does not already tell you. Therefore, the variable does not add as much. The debugging argument could still hold, but again, in this specific case I don't see anything that is likely to be your focus when debugging, and it can always be easily changed later if somehow someone needs that format for debugging or anything else.

In general, and you did ask for the general rule (your example was just that, an example of a generalized form), all the points made in favor of variant 1 (2-liner) are correct. Those are good guidelines to have. But guidelines need to be flexible. For example, the project I'm working on now has an 80 character per line maximum, so I split a lot of lines, but I commonly find lines 81-85 characters which would be awkward to split or reduce readability and I leave them over the limit.

Since it's unlikely to add value, I would not do 2 lines for the specific example given. I would do variant 2 (the 1-liner) because the points are not strong enough to do otherwise in this case.

Answer 8

I would argue that a 3rd option would be better and easier to read and reason about than either of the two you presented:

private string GetFormattedValue()
{
    return format != null 
        ? string.Format(format, value) 
        : value.ToString();
}

Your co-worker's point about it being easier to debug has merit, but typically if I need to see the return value before returning I modify the code, and then change it back to omit the temporary variable after debugging.

OTOH it would be great if Visual Studio — and the JetBrains IDEs I use instead — would provide an option to see the value to be returned after the expression is executed but before it is actually returned. #fwiw