Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Golden Path #63

Closed
JRG-Developer opened this issue Nov 4, 2014 · 17 comments
Closed

Golden Path #63

JRG-Developer opened this issue Nov 4, 2014 · 17 comments
Assignees

Comments

@JRG-Developer
Copy link
Member

To steal from our Objective-C style guide, I propose the inclusion of the Golden Path rule in this guide:

Golden Path

When coding with conditionals, the left hand margin of the code should be the "golden" or "happy" path. That is, don't nest if statements. Multiple return statements are OK.

Preferred:

func someMethod() {

  if someBoolValue == false {
    return
  }

  // Do something important
}

Not Preferred:

func someMethod() {

  if someBoolValue == true {
    // Do something important
  }
}
@gregheo
Copy link
Contributor

gregheo commented Dec 3, 2014

I would agree here but what about optional binding? Sometimes there are a big mess of if let cases stacked up with the "happy" code way down in all the curly braces.

I suppose we could replace that with many checks for nil, but that seems against the Swift way.

@cwagdev
Copy link

cwagdev commented Dec 3, 2014

Checking for nil is definitely going against swift in my opinion. Then when you're done you're left to use forced unwrapping which feels even worse. 


Chris

On Tue, Dec 2, 2014 at 6:54 PM, Greg Heo notifications@github.com wrote:

I would agree here but what about optional binding? Sometimes there are a big mess of if let cases stacked up with the "happy" code way down in all the curly braces.

I suppose we could replace that with many checks for nil, but that seems against the Swift way.

Reply to this email directly or view it on GitHub:
#63 (comment)

@JRG-Developer
Copy link
Member Author

I think the scenario we should discourage is nesting a big mess of if let statements.

E.g.

if let object1= optional1 {
  if let object2 = optional2 {
     if let object3 = optional3 {
        // important code is nested deep inside here...
     }
   }
} else {
  // handle missing values
}

Seems bad.

Instead, I propose this is cleaner:

if optional1 == nil || optional2 == nil || optional3 == nil {
  // handle missing values
  return;
}

let object1 = optional1!
let object2 = optional2!
let object3 = optional3!

// important code is aligned with the left

@rwenderlich
Copy link
Member

Hm - I worry about that second approach - it seems more error-prone than nested if lets. What if someone adds a let object4 = optional4! but forgets to add it in the if statement? Also I think ! should be avoided as much as possible, we shouldn't be encouraging it...

@JRG-Developer
Copy link
Member Author

If they added another let object4 = optional4! without adding it to the if statement, you would hope that the app would crash quickly, but I can see how this may persist as a silent bug if the input is present most of the time...

Maybe there isn't a good solution to this issue, and it should be addressed on a case by case basis?

For example, start by questioning, "Why do I have four optionals here...?"

Unless others have a better suggestion on how to address this, perhaps this should simply be closed...?

@gregheo
Copy link
Contributor

gregheo commented Dec 9, 2014

I'd worry a little bit too, especially if there might be "in between" work to do, e.g.:

if let object1= optional1 {
  // do something with object1 here
  if let object2 = optional2 {
     if let object3 = optional3 {
       // do something with objects 1,2,3
     }
   }

I guess you could split that up into two if blocks, one for just object1 and another for all of 1,2,3.

I feel like Swift is going to get multiple bindings some day, like this:

if let object1 = optional1 && object2 = optional2 {
}

In that case, I'd have to argue against the usual golden path and say it's an established Swift idiom to have the "happy path" inside the conditional.

@JRG-Developer
Copy link
Member Author

Closing this issue as the group seems to agree that the Golden Path has undesirable consequences in Swift.

As @gregheo mentions, hopefully Swift will have multiple let bindings included one day. When such day comes, perhaps such can be recommended to be added to the guide.

:]

@JRG-Developer
Copy link
Member Author

With the introduction of guard in Swift 2.0, I think we should consider adding the Golden Path to our Swift guide.

I think many agree this is a good idea in Swift now and are already following it in their writing.

@JRG-Developer
Copy link
Member Author

Also, multiple guard / let bindings are now allowed by putting a comma between them. :]

@lukewar
Copy link

lukewar commented Dec 30, 2015

I agree. Example from above in Swift 2.0 should look more like this:

guard let optional1 = optional1, let optional2 = optional2, let optional3 = optional3 else {
    // handle missing values
    return;
}

// important code is aligned with the left

@rayfix
Copy link
Contributor

rayfix commented Apr 7, 2016

Approved. I will be adding a section about golden path and guard in the style guide. @lukewar I like your example but ... that semicolon ... 😆

@lukewar
Copy link

lukewar commented Apr 7, 2016

lol, jumping between Swift and ObjC is harder than one would think :P

@RobertGummesson
Copy link

How about dropping the let duplicates from that example? You guys think they add clarity?

guard let optional1 = optional1, optional2 = optional2, optional3 = optional3....

@rayfix rayfix self-assigned this Apr 7, 2016
@JRG-Developer
Copy link
Member Author

I'm against dropping the let duplicates, or at least, making a hard rule there shouldn't be any duplicates.

Reasoning:

(1) If you include a where clause, you must specify let again. For example, this is a problem:

Not Allowed:

func doSomethingWithOptionalNumber(number1: Int?, number2: Int?) {

  guard let number1 = number1 where number1 > number2,
    number2 = number2 else {   //  COMPILER ERROR
      print("Number criteria not met!")
      return
  }

  // ... here be magic...
}

(2) If you have a mix of var and let, it can be unclear what your intentions are if you omit the let/var specifier:

Potentially Unclear:

guard let number1 = number1,
  var number2 = number2,
  number3 = number3 else { // Did you really mean for this to be `var` or actually `let`?
    return
}

I'd propose:

  • There should always be a let/ var, even if redundant.
  • Or, the decision should be left up to the tutorial author, depending on context.

@rayfix
Copy link
Contributor

rayfix commented Apr 7, 2016

Use of var is deprecated (and will be an error in Swift 3) so that argument is out. Also, if you order correctly you can omit the let with where.

func doSomethingWithOptionalNumber(number1: Int?, number2: Int?) {

  guard let number1 = number1, number2 = number2 where number1 > number2
    else {   //  COMPILER ERROR
      print("Number criteria not met!")
      return
  }

  // ... here be magic...
}

That said, I understand that a style guide shouldn't be a straight jacket. While I may elide the let from the example, we don't have to necessarily rule on it.

@JRG-Developer
Copy link
Member Author

@rayfix Admittedly, these were contrived examples. 😉

However, you definitely won't always be able to reorder to get rid of the necessity for the second let.

Regarding,

Use of var is deprecated

You can only have let variables now?! That's definitely quite a shift to functional programming in Swift!

😱 😱 🙀

Jokes aside though, only var method parameters are deprecated. You can definitely have var in a guard statement. Here's an example:

func addDefaultHeadersTo(headers: [String: String]?) -> [String: String] {

  let defaultHeaders = ["AppVersion": "1.0.42",
                        "Who's Awesome?": "Ray Fix"]

  guard var headers = headers else {
    return defaultHeaders
  }

  for (key, value) in defaultHeaders {
    headers[key] = value
  }

  return headers
}

@JRG-Developer
Copy link
Member Author

That said, I understand that a style guide shouldn't be a straight jacket. While I may elide the let from the example, we don't have to necessarily rule on it.

👍

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

No branches or pull requests

7 participants