-
Notifications
You must be signed in to change notification settings - Fork 0
[FIX] update the guidelines for enumeration switch cases #26
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,67 @@ enum Shape { | |
| } | ||
| ``` | ||
|
|
||
| ## Switch Statements | ||
| 1. Enum cases with an associated value should have a appropriate label as opposed to just types | ||
|
|
||
| ***Preferred:*** | ||
|
|
||
| ```swift | ||
| enum Trade { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| case buy(stock: String, amount: Int) | ||
| case sell(stock: String, amount: Int) | ||
| } | ||
| ``` | ||
|
|
||
| ***Not Preferred:*** | ||
|
|
||
| ```swift | ||
| enum Trade { | ||
| case buy(String, Int) | ||
| case sell(String, Int) | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| 2. When using a switch statement that has a finite set of possibilities, do not include a _default_ case. Instead, place unused cases at the bottom and use the break keyword to prevent execution. | ||
|
|
||
| ```swift | ||
| enum UserState { | ||
| case loggedIn | ||
| case loggedOut | ||
| case openCatalogue | ||
| case onboarding | ||
| } | ||
| ``` | ||
|
|
||
| ***Preferred:*** | ||
|
|
||
| ```swift | ||
| func handleUser(with state: UserState) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not 100% agreed for this to be a guideline. I think it depends on the enum specially if the other options are completely irrelevant to what the switch is trying to address. Another example would be you have an error enum with multiple errors that and you can only act on a handful it becomes boilerplate to enumerate all of them just to follow a rule. For example something that I'm working for a personal project can throw this error https://developer.apple.com/documentation/networkextension/nehotspotconfigurationerror enum LoginResult {
case loading
case success
case error
}Make complete sense to have no default statement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our own
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this recommendation could be relaxed a bit? There is a lot of value in not using the |
||
| switch state: | ||
| case .loggedIn: | ||
| showMainUI() | ||
| case .onboarding: | ||
| showOnboardingFlow() | ||
| case .openCatalogue, .loggedOut: | ||
| showVisitorUI() | ||
| } | ||
| ``` | ||
|
|
||
| ***Not preferred:*** | ||
|
|
||
| ```swift | ||
| func handleUser(with state: UserState) { | ||
| switch state: | ||
| case .loggedIn: | ||
| showMainUI() | ||
| case .onboarding: | ||
| showOnboardingFlow() | ||
| case default: | ||
| showVisitorUI() | ||
| } | ||
| ``` | ||
|
|
||
| ### Prose | ||
|
|
||
| When referring to functions in prose (documentation, code comments, code reviews) include the required parameter names from the caller's perspective or `_` for unnamed parameters. Examples: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence needs a
.at the end 😉 .Also, what is the rationale for this recommendation? It should be added as well.