Skip to content
This repository was archived by the owner on Oct 9, 2018. It is now read-only.

Moving braces to the next line increases readability in some situations #19

Open
ghost opened this issue Jul 11, 2014 · 6 comments
Open

Comments

@ghost
Copy link

ghost commented Jul 11, 2014

The guidelines say "Opening braces always go on the same line.", but in some situations (complex match inputs or iterator chains), readability increases when the opening curly bracket is moved to the next line:

Compare these two

for (x, y) in range(0u, 3)
        .flat_map(|e| Repeat::new(e).zip(range(0u, 3)))
        .flat_map(|e| Repeat::new(e).zip(OFFSETS.iter()))
        .map(|((x,y), &(x1,y1))| (x + x1, y + y1)) {
    grid.get_mut(y)
        .and_then(|row| row.get_mut(x))
        .map(|tile| *tile += 1);
}

for (x, y) in range(0u, 3)
    .flat_map(|e| Repeat::new(e).zip(range(0u, 3)))
    .flat_map(|e| Repeat::new(e).zip(OFFSETS.iter()))
    .map(|((x,y), &(x1,y1))| (x + x1, y + y1))
{
    grid.get_mut(y)
        .and_then(|row| row.get_mut(x))
        .map(|tile| *tile += 1);
}

Or

match (rng.gen::<bool>(),
        rng.gen::<bool>(),
        rng.gen::<bool>()) {
    (true,true,true) => { ... },
    ...
}

match (rng.gen::<bool>(),
    rng.gen::<bool>(),
    rng.gen::<bool>())
{
    (true,true,true) => { ... },
    ...
}
@nielsle
Copy link

nielsle commented Jul 11, 2014

I like version 1 with a newline:

for (x, y) in range(0u, 3)
        .flat_map(|e| Repeat::new(e).zip(range(0u, 3)))
        .flat_map(|e| Repeat::new(e).zip(OFFSETS.iter()))
        .map(|((x,y), &(x1,y1))| (x + x1, y + y1)) {

    grid.get_mut(y)
        .and_then(|row| row.get_mut(x))
        .map(|tile| *tile += 1);
}

@dyrim
Copy link

dyrim commented Jul 11, 2014

I think maybe in those cases it would be better to extract the long chain into a separate method?

@aturon
Copy link
Member

aturon commented Jul 14, 2014

FYI: I've added a link to this issue on discuss.rust-lang.org.

@lilyball
Copy link
Contributor

The indentation in both of those examples is wrong. This is what it should look like:

for (x, y) in range(0u, 3)
              .flat_map(|e| Repeat::new(e).zip(range(0u, 3)))
              .flat_map(|e| Repeat::new(e).zip(OFFSETS.iter()))
              .map(|((x,y), &(x1,y1))| (x + x1, y + y1)) {
    grid.get_mut(y)
        .and_then(|row| row.get_mut(x))
        .map(|tile| *tile += 1);
}

The extra indentation on the line continuations makes it a bit more distinct from the body of the block.

Your second example doesn't get the extra indentation though (but you still misindented):

match (rng.gen::<bool>(),
       rng.gen::<bool>(),
       rng.gen::<bool>()) {
    (true,true,true) => { ... },
    ...
}

Personally, I think putting the opening brace on the same line is just fine in both cases (though really, that second case doesn't even need line continuations at all).

I would say that if the line continuations are so close in indentation level to the body of the block that it's confusing, then it might make sense to put the brace on a new line, but in that case I suspect that you probably just misindented or picked a bad spot to break the line. Normally the way the line continuation lines up with the previous lines makes it visually very obvious that it's a continuation, and that makes it easy to tell where the body of the block starts.

Given that, I think it's probably better just to leave the guideline as it is. In truly exceptional cases, people can choose to break the guideline, but in the vast majority of cases it's better to just leave it as a simple straightforward rule.

@chris-morgan
Copy link
Member

I contend that that first example should actually be formatted like this—

for (x, y) in range(0u, 3).flat_map(|e| Repeat::new(e).zip(range(0u, 3)))
                          .flat_map(|e| Repeat::new(e).zip(OFFSETS.iter()))
                          .map(|((x,y), &(x1,y1))| (x + x1, y + y1)) {
    grid.get_mut(y)
        .and_then(|row| row.get_mut(x))
        .map(|tile| *tile += 1);
}

Visual indent is a good thing.

Does it seem too long a line? Well, that’s not my fault—the starting code is convoluted and should be refactored to make more sense.

As for the match example, the whole ::<bool> thing shouldn’t be there anyway, and then a single line is entirely reasonable…

Such complicated examples as these are not good examples that should permit an exception to the rules or a weakening of the rules—they are the reason for the rules. If, when formatted like this, your code is ugly and/or hard to read, you should probably rewrite it so that it won’t be. It will normally be easier to read and reason about afterwards.

@lilyball
Copy link
Contributor

@chris-morgan You're right, I didn't want to try to measure line length in the GitHub comment editor, but that first line as you have it is only 73 characters, so it should indeed be wrapped/indented in the fashion you describe.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants