ustwo / objective-c-style-guide

The ustwo Objective-C style guide
20 stars 5 forks source link

Add segment about simple if statements with return #4

Open KATT opened 10 years ago

KATT commented 10 years ago

More readable to do...

.. this
- (void)twoSidedViewWillFlip:(US2TwoSidedView *)view
{
    if (!view.isFrontFacing) return;

    // [..]
}
.. rather than this
- (void)twoSidedViewWillFlip:(US2TwoSidedView *)view
{
    if (!view.isFrontFacing)
    {
        return;
    }

    // [..]
}
martinstolz commented 10 years ago

:+1:

alexfish commented 10 years ago

:thumbsup:

iceydee commented 10 years ago

Such a ruby dev.

Don't we open a can of worms by allowing that?

KATT commented 10 years ago

Don't we open a can of worms by allowing that?

Why?

iceydee commented 10 years ago

After that comes:

if (something)
  do this
else if
  do this
else
  do this third

And then we will have the inevitable (bug):

if (something)
  do this
else if
  do this
else
  do this
  do this, expect to only happen in else, but will actually always get executed.
KATT commented 10 years ago

No. Because no sane person does

if (cat)
    meow();
else 
    woff();

or at least, I don't think we should allow it.

iceydee commented 10 years ago

I don't mind your example. If the return is the only thing that is happening.

Must be very clear that this is the only case where we allow no braces though.

svoctor commented 10 years ago

In theory I agree, in practice I don’t.

No edge cases is better than one edge case.

KATT commented 10 years ago

In blocks for instance, basically any case an edge case according to style guides:

There are several appropriate style rules, depending on how long the block is:

  • If the block can fit on one line, no wrapping is necessary.
  • If it has to wrap, the closing brace should line up with the first character of the line on which the block is declared. Code within the block should be indented four spaces.
  • If the block takes no parameters, there are no spaces between the characters ^{. If the block takes parameters, there is no space between the ^( characters, but there is one space between the ) {characters.
alexfish commented 10 years ago

I think we should allow this edge case, it's really handy in blocks, e.g in LR:

typeof (self) __strong strongSelf = weakSelf;
if (!strongSelf) return;
madhikarma commented 10 years ago

:+1:

Return early this way is ok for me as well.

Note. I don't like returning early inside if-else branches though, should have a variable and return that at the end

iceydee commented 10 years ago

@KATT The blocks example isn't a very good one since we've just put in writing exactly how XCode wants to format it.

KATT commented 10 years ago

XCode is an edge case.

alexfish commented 10 years ago

Any progress on this?

KATT commented 10 years ago

Seemed like a majority was in favour, do a pull request?

alexfish commented 10 years ago

yeah go for it

iceydee commented 10 years ago

Be very clear about when this is acceptable though.

OSX and iOS has had non-functioning SSL because of non-bracket if-clauses. We must never let those in.

KATT commented 10 years ago

They separated the condition and action on multiple lines, it wouldn't happen using the above example.

Their bug:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
// [there probably was an if-statement here previously]
    goto fail;  /* MISTAKE! THIS LINE SHOULD NOT BE HERE */
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

If they followed the above convention they would've deleted the goto fail when deleting the if-statement too.

:smiley_cat:

(yes, you have a good point)

KATT commented 10 years ago

Maybe the condition is too edgy to be in the style guides. Opinions?