resource / Front-End-Standards

Front-end development standards for Resource.
MIT License
23 stars 1 forks source link

if/else block formatting #24

Closed adamweis closed 11 years ago

adamweis commented 11 years ago

It looks like this was brought up before in #20, but I don't see a reasoning as to why the new line version is better than the other for readability.

I've personally always used the } else { format and it seems to be the most widely accepted way to format an if/else, even appears that way in the idomatic.js.

JohnSpadafore commented 11 years ago

I've typically been doing the } else { format as well, seems to be the better option for readability.

LukeAskew commented 11 years ago

So here is my reasoning:

Typically, you write comments on the preceeding line. E.g:

// This is a comment
if (comment) {
    // ... do stuff ...
}

but if you want to comment the else block consistently, and its formatted as } else {, then it would look like:

// This is a comment
if (comment) {
    // ... do stuff ...
// comment
} else {
    // ... do stuff ...
}

So most people end up doing:

// This is a comment
if (comment) {
    // ... do stuff ...
} else { // comment
    // ... do stuff ...
}

which is inconsistent. Therefore, the consistent way of commenting blocks is:

// This is a comment
if (comment) {
    // ... do stuff ...
} 
// comment
else { 
    // ... do stuff ...
}

This brings up another issue of "avoiding EOL comments", which I don't think we've covered.

I believe new line comments increase legibility because the top-to-bottom logic flow is easier to follow.

JohnSpadafore commented 11 years ago

I can't think of that many situations that would require use of a comment before an else, unless it were an else if or a nested statement.

// This is a comment
if (comment) {
    // ... do stuff ...
// comment
} else {
    // ... do stuff ...
}

Even then, your first example looks to be the cleanest one.

LukeAskew commented 11 years ago

I do it all the time.

// if conditions XYZ, then do this
if (XYZ) {
    // this
}
// otherwise, do that
else {
    // that
}
JohnSpadafore commented 11 years ago

Has it been standard for other developers as well? I honestly haven't seen that many people use comments for else statements unless defining what if statement it is connected to.

JBRector commented 11 years ago

I also prefer

} else {

If I do need to comment the else, I typically write it like John has it above, but with an extra line above the else comment like so:

// This is a comment
if (comment) {
    // ... do stuff ...

// comment
} else {
    // ... do stuff ...
}
nicetransition commented 11 years ago

I do the

} else {
    // code here

All comments for a conditional statement, I put in a comment block before the if

nicetransition commented 11 years ago

If you have a complex conditional that requires explanation for the else, why shouldn't it just go in the top comment block?

JBRector commented 11 years ago

That makes sense.

apartyka commented 11 years ago

I agree with Kevin here. One block of comments to explain the if/else.

If your first comment is written like...

// if conditions XYZ, then do this

...No need to break out a separate "else" comment when it's sufficient to explain it at the If level. The "then do this" portion is your "else" block.

DRY up the comment, so to speak.

LukeAskew commented 11 years ago

The purpose of commenting the block is two-fold:

  1. Explain the evaluation.
  2. Give an overview of the subsequent functions.

E.g.

// if Kevin is here, then give him work
if (kevinsLocation === "here") {
    assignTasks();
    emailUser();
    eatSandwich();
}
// if he's not here, shame him
else {
    kickDirt("face");
    shame();
}

@mcgaryes what are your thoughts?

nicetransition commented 11 years ago

But isn't that the conditional? If Kevin is here then do X if not don't, why explain both?

Complex conditionals should be handled by a function or strings for arguments for clarity... right?

nicetransition commented 11 years ago

BTW - I love this debate :)

(I love to see us all talking about our standards, it's so fucking awesome!)

Krxtopher commented 11 years ago

Like others, I have a hard time imagining a situation where the meaning of "else" wouldn't be self evident if the "if" (and "if else") parts are clear.

But how to comment "if else" is something that's still worth discussing. That said, the format Luke proposes is far outside the normal options people use for line breaks concerning "else" and "if else". For that reason alone I'd avoid it.

nicetransition commented 11 years ago

I agree. Sorry, Luke. womp womp

I believe that even with else if it should also live in a multiline code block above the conditional block.

nicetransition commented 11 years ago

@LukeAskew, I see you removed the block but we should add in example

 if ( /*conditional*/ ) {
    // do this
} else {
  // or this
}

Maybe even provide an example in comments:

/**
 * When `shoe` is tied (`true`), alert the users with a message.
 * If `shoe` is not tied or `undefined`, begin the process over again.
 */
 if ( shoe.isTied ) {
    alert("Great job!");
} else {
    tieShoeLace();
}
LukeAskew commented 11 years ago

@subzthanabalan @mrbinky3000 You two have the final say on this.

What do you think of the standard? Should we add additional examples per @kevinmack18 suggestion?

subzthanabalan commented 11 years ago

Considering the lengthy debate, I would say we provide an example with comments.

Krxtopher commented 11 years ago

In line with Subathra's suggestion, I've always like code style guides that provide a single complete example that demonstrates all the recommendations in action. What would you guys think of that? I'm thinking this would be in addition to the mini examples inline in the doc.

Google does this at the start of their Objective-C Style Guide. They introduce it this way:

They say an example is worth a thousand words so let's start off with an example that should give you a feel for the style, spacing, naming, etc.

nicetransition commented 11 years ago

I really like that idea, but I think that should belong in a separate ticket

lkrids commented 11 years ago

This is more difficult to read or at least pick out

  lines of code and comments
} else {
  lines of code and comments

than

  lines of code and comments
} 
else {
  lines of code and comments


typically, JSON is written like this:

var t = [
  {
    ...
  },
  {
    ...
  }
]

not

var t = [
  {
    ...
  }, {
    ...
  }
]