26

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?

  • 18
    Not working on VS, but would assume you cannot set a conditional breakpoint on a complicated expression (or it would be complicated to enter), so would probably put assign and return into separate statements, just for convenience. The compiler would very probably come up with the same code for both anyhow. – tofro Dec 18 '17 at 7:45
  • 1
    This is possibly language-dependent, especially in languages where variables have (possibly complex) object behavior instead of pointer-behavior. @Paul K's statement is probably true for languages with pointer behavior, languages where objects have simple value behavior, and languages with mature, high-quality compilers. – MSalters Dec 18 '17 at 10:57
  • 4
    "since VisualStudio allows us a very detailed inspection of the statements, when the execution is stopped due to a break point" - is that so. So how do you get the return value if the function returned a struct with more than one member? (and the support for that feature is spotty at best, there's a lot of combinations where you don't get the return value at all). – Voo Dec 18 '17 at 12:45
  • 2
    How does having " very detailed inspection of the statements" in the debuger someone is using TODAY, make it a bad option to write the code so it is easy to debug in ANY debuger? – Ian Dec 18 '17 at 15:42
  • 16
    Annoy him further by reducing the entire function body to private string GetFormattedValue() => string.Format(format ?? "{0}", value); – Graham Dec 18 '17 at 17:13
47

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.

  • I agree, too, that there definitely are cases in which it's useful, no doubt. – Paul Kertscher Dec 18 '17 at 7:27
  • 2
    I usually use result as the name of the variable. Not so much longer and easier to debug – edc65 Dec 18 '17 at 9:59
  • 26
    @edc65: a generic name like result often just adds noise to the code and does seldom increase readability, which is exactly the point of my answer. This can be justified in context where it helps debugging, but I would avoid when using a debugger who does not need a separate variable. – Doc Brown Dec 18 '17 at 11:37
  • 6
    @JonHanna way tool long according to me. The name result conveys the information that this is the value resulting from function, so that you can look at it before the function returns. – edc65 Dec 18 '17 at 15:56
  • 1
    @edc65 but that makes it look useful. So now when I'm reading your code I don't immediately realise it's not. So your code has become less readable. – Jon Hanna Dec 18 '17 at 16:00
40

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.

  • 9
    This is the correct answer for me. This makes it easier to set a breakpoint and see the value when debugging, and has no downside that I’m aware of. – Matthew James Briggs Dec 18 '17 at 16:31
  • For point b, in Visual Studio Code, just put breakpoint on the return and then add the expression: GetFormattedValue() and that will show the result when the breakpoint is hit, so the extra line is not needed. But it is easier to see the locals with extra line as it will not require adding any additional expression in the debugger. So, really a matter of personal preference. – Jon Raynor Dec 18 '17 at 19:04
  • 3
    @JonRaynor for the return value, put the breakpoint on the closing bracket of the function. It catches the returned value, even in functions with multiple returns. – Baldrickk Dec 19 '17 at 11:08
16

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”.

  • 1
    "I find both solutions confusing because the conditional operator is being used unnecessarily." — It's not a real world example, I just had to make something up, quickly. Admittedly this might not be the best example. – Paul Kertscher Dec 18 '17 at 7:28
  • 4
    +1 for essentially saying this is an "under the radar" difference that (other things being equal) it's not worth being that bothered about. – TripeHound Dec 18 '17 at 13:09
  • 3
    @Mindwin, when I use the ternary operator, I usually split it into multiple lines so that it's clear what's the true case and what's the false case. – Arturo Torres Sánchez Dec 18 '17 at 15:30
  • 2
    @ArturoTorresSánchez I do that too, but instead of a ? and a : I use if() { and } else { - - - - \\ :) – Mindwin Dec 18 '17 at 15:58
  • 3
    @Mindwin, but I can't do that when I'm in the middle of a expression (like an object initializer) – Arturo Torres Sánchez Dec 18 '17 at 16:11
2

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)

1

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.

  • 1
    The first example seems bad practice because value.ToString() gets called unnecessarily when format is non-null. In the general case, this could include non-trivial calculations, and may take longer than the version including a format string. Consider, for example, a value that stores PI to a million decimal places, and a format string that requests only the first few digits. – Steve Dec 19 '17 at 9:48
  • 1
    why not private string GetFormattedValue() => string.Format(format ?? "{0}", value); Same affect, and use unit tests to ensure correctness instead of relying on the debugger. – Berin Loritsch Dec 19 '17 at 13:04
  • 1
    While I agree a ternary can be less clear, the null terminator can make things more clear. At least in this case. – Berin Loritsch Dec 19 '17 at 13:37
  • 1
    Dear diary, today I've read that writing clear and concise code using well-known (existing for about 40 years) paradigms, idioms and operators is, quote, double quote being clever double quote, unquote - but instead writing overly verbose code violating DRY and not using aforementioned operators, idioms and paradigms while instead trying to avoid anything that could possibly seems cryptic only to a five year old with no prior programming background - is clarity instead. Hell, I must've gotten real old, my dear diary... I should've learned Go when I had the chance. – vaxquis Dec 19 '17 at 14:52
  • 1
    "Using ternary operators for more complex logic would be unreadable" While it is indeed the case (and I have seen people overcomplicate logic) this is not true for the OP's code, and is also not a specific thing to ternary operators. The only thing I can say with full confidence is that the line is too long. gist.github.com/milleniumbug/cf9b62cac32a07899378feef6c06c776 is how I would reformat it. – milleniumbug Dec 19 '17 at 21:02
1

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?

  • Martin Fowler is an intelligent man and I enjoyed reading his (and your) views. While I firmly believe that testing is necessary and that more time should be spent in that endeavor, the fact that we are all fallible human beings suggests that no amount of testing will eradicate all bugs. Hence, debugging will always be a part of the program development and support process. – tale852150 Dec 22 '17 at 17:47
1

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.

protected by gnat Dec 22 '17 at 4:37

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

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.