Newline before else

[repax]

Newline before else / else if

Should there be a newline before else?

Snippet from the list.rs example file

let post_snippet_trimmed = if post_snippet.starts_with(',') {
    post_snippet[1..].trim_matches(white_space)
} else if post_snippet.ends_with(',') {
    post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space)
} else {
    post_snippet
};

vs:

let post_snippet_trimmed = if post_snippet.starts_with(',') {
    post_snippet[1..].trim_matches(white_space)
}
else if post_snippet.ends_with(',') {
    post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space)
}
else {
    post_snippet
};

Some specifics from the Guiding principles

I found some guidelines that might be relevant:

• compatibility with version control practices - preserving diffs, merge-friendliness, etc.
• preventing right-ward drift
• minimising vertical space

Would a newline be desirable with regards to version control logs when adding / removing else blocks? If so, does that outweigh the benefit of minimising vertical space -- a point of lower priority according to the list of guiding principles?

Does a newline here, in any way, influence readability or scan-ability? For example, does the vertical space help perception in grouping lines of code that belong together (i.e. the condition and the subsequent block contents)?

[joshtriplett]

I don't believe that a newline between the closing brace and the else would affect version control diffs one way or another. Rust requires braces around the body of an if or else (unlike C, which allows omitting them for a single-statement body), so a diff would never need to delete the brace without the else, and removing or changing the else doesn't require altering the brace. So, I can't think of a diff scenario where a change to one line would require changing an unrelated line (as in the trailing comma case, where without a trailing comma a change to one line could require changing the punctuation on another line that doesn't otherwise need changing).

I also don't think that newline would help with grouping, for the same reason that the concept of else if exists: if ... { body } else { body2 } represents a multi-way conditional, not two independent constructs with associated blocks. The } else { provides both the end of the first block of that multi-way conditional and the start of the next; personally, I read } else { almost as a single semantic unit.

[nrc]

I don't find that a newline here helps scanability - I'm looking at the shape of the code then and the blocks/indenting make the shape obvious, you don't need any further help finding the else.

I agree with Josh about the diff stuff.

I don't think this significantly affects rightward drift, usually when we care about rwd, we are talking about half a line, or most of a line in edge cases, this change would only save two characters. Furthermore, since we never have anything after the {, it will always be a short line and the extra two characters won't help.

Finally, as well as adding an extra line, I think this makes the code noisier by making two short lines instead of one short line.

Overall, I'm not in favour.

[phaylon]

I personally do prefer the newline.

The first reason would be consistency between collapsed and expanded block versions:

if condition { result }
else { default }

versus

if condition {
    // longer content
}
else {
    // longer content
}

I also like that everything stays consistent even if I end up mixing expanded/oneline versions. Usually when every branch returns a simple value, and the else constructs an error.

The second reason immediately coming to mind looking at my code is that I'd certainly have the newline when I add a comment:

if 23 == 23 {
    // yay
}
// panic note: this can only happen if math stopped working
else {
    panic!("math error");
}

A version to do that without a newline is to put it in the previous block:

if 23 == 23 {
   some_code();
// panic note: ...
} else {
   panic(...);
}

but everytime I see this with longer comments it looks like there's a closing brace missing before the comment.

If you use the no-newline version by default and add a newline for the comment, you end up with a mixed result like this:

if cond_1 {
    // content
} else if cond_2 {
    // content
} else if cond_3 {
    // content
}
// some comment
else if cond_4 {
    // content
} else {
    // content
}

which I find gives an odd grouping effect that is unintended.

[joshtriplett]

@phaylon The Rust style doesn't support "collapsed" conditionals like that either. If the entire conditional (if and else) doesn't fit on one line, the style guide recommends splitting out each block.

For comments, I tend to either put them inside the block, or above the if, or (rarely) on the same line as the else and braces.

[strega-nil]

@joshtriplett @phaylon I personally prefer "on the same line":

if { // foo
} else { // bar
}

[est31]

I personally prefer else to be on the same line, but I think that there should be an option to customize this.

[nrc]

FCP, propose to close

[KalitaAlexey]

Hi @est31,

Why should such thing be customizable?

[est31]

@KalitaAlexey why not?

Its popular in C where many people are coming from.

[KalitaAlexey]

@est31, pointers are popular in C, but they shouldn't use them right?
I just don't understand why do we need any customization at all.
Preferred style is habit.
I am able to write in different styles for different languages.
I don't try to write like I like where this violates conventions.

[est31]

@KalitaAlexey your comment better fits to this pr: #33

[KalitaAlexey]

@est31, yes thanks.

[joshtriplett]

@est31 I've seen two popular styles in C:

if (condition) {
    body;
} else {
    body;
}

and

if (condition)
{
    body;
}
else
{
    body;
}

(I've also seen some software that indented the braces to match the body.)

I've seen perhaps a couple of codebases, ever, that put a newline before the else but put opening braces on the same line as if and else.

[strega-nil]

@KalitaAlexey otoh, I use the same style literally everywhere (as close as possible; i.e., in python I don't deal with braces, in C++ I don't have trailing commas). I can empathize with people who want to continue using their preferred style in Rust.

[Mark-Simulacrum]

@ubsan Just to clarify, you prefer the style without a newline before the else, correct?

My reason for preferring style 1, below, is because it reduces the amount of vertical space used for "nothing". There is no reason or need to give the braces an entire line (or 4, in the completely expanded case). However, I do see that a style like style 2 could be preferred by some since it keeps all conditions aligned. I don't think this alignment is worth it in Rust, especially since the blocks in between can be large enough that the alignment is "worthless", i.e. it doesn't have any effect since both cannot be seen simultaneously or scanned between visually.

Style 1:

if condition {
    body;
} else {
    body;
}

Style 2:

if condition {
    body;
} else
if condition {
    body;
}

[strega-nil]

@Mark-Simulacrum I don't put the whitespace, but I can empathize with people who do. I don't know if it's 'worthless' or w/e, I think it's fine. I probably did that at some point, knowing how many different styles I've used over the years, before finally settling on this one. I do very, very much dislike style 2, and I don't think I've ever seen it used; much more common is the else if on one line, below the }

[repax]

Each choice has its benefits, of course. What bothers me the most is the wall of code in the first example I presented. I find that a newline between branches seems to help open up the text sometimes.

For comparison, here's what the match expression might look like using the analogical vertical contraction:

match lit.node {    
    ast::LitKind::Int(_, ast::LitIntType::Unsigned(_)) => {
        forbid_unsigned_negation(cx, e.span);
    } ast::LitKind::Int(_, ast::LitIntType::Unsuffixed) => {
        if let ty::TyUint(_) = cx.tcx.tables().node_id_to_type(e.id).sty {
            forbid_unsigned_negation(cx, e.span);
        }
    } _ => (),
}

instead of the more commonly used style:

match lit.node {    
    ast::LitKind::Int(_, ast::LitIntType::Unsigned(_)) => {
        forbid_unsigned_negation(cx, e.span);
    }
    ast::LitKind::Int(_, ast::LitIntType::Unsuffixed) => {
        if let ty::TyUint(_) = cx.tcx.tables().node_id_to_type(e.id).sty {
            forbid_unsigned_negation(cx, e.span);
        }
    }
    _ => (),
}

[nrc]

I think this has been in FCP long enough with no new discussion. Closing. We may well allow an option to do this, but that should be discussed on an if else RFC.