-
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?
Conversation
mitulmanish
commented
Aug 17, 2018
- Adds new guidelines for enumeration switch cases
martinpilch
left a comment
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.
Makes sense to me. It makes code safer 👍
| ***Preferred:*** | ||
|
|
||
| ```swift | ||
| func handleUser(with state: UserState) { |
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.
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
Of course there are different opinions and as usual there is a balance to achieve as well as preferences are involved so if the rest of the team is ok with this I'll adhere.
Cases like
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Our own NetworkServiceError is another example.
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.
Perhaps this recommendation could be relaxed a bit?
There is a lot of value in not using the default case, but there are as @pablocaif suggests, instances in which it would be too cumbersome to do.
| ***Preferred:*** | ||
|
|
||
| ```swift | ||
| enum Trade { |
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.
👍
| ``` | ||
|
|
||
| ## Switch Statements | ||
| 1. Enum cases with an associated value should have a appropriate label as opposed to just types |
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.
mokagio
left a comment
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.
Great initiative, thanks! A few comments.