-
Notifications
You must be signed in to change notification settings - Fork 1
Duru/readd and fix #47
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: main
Are you sure you want to change the base?
Conversation
|
|
||
| // Both | ||
| // case Basketball | ||
| case Basketball |
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.
can we clean this up a bit? I think a cleaner approach would look closer to this:
enum Sport: String, Identifiable, CaseIterable, CustomStringConvertible {
var id: Self { self }
case all = "All"
case basketball = "Basketball"
case iceHockey = "Ice Hockey"
case lacrosse = "Lacrosse"
case soccer = "Soccer"
case fieldHockey = "Field Hockey"
case baseball = "Baseball"
case football = "Football"
}
| case All | ||
|
|
||
| // Both | ||
| // case Basketball |
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.
we should remove the comments as well
| case .Baseball: return game.timeUpdates.count > 3 ? game.timeUpdates.count - 2 : 10 | ||
| // the last three columns are total runs, hits, and errors | ||
| // if backend stores null for scoreBreakdown, display regular score box with 10 columns | ||
| case .Basketball: |
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.
No need for comments for each sport, you should remove those. Additionally, you should be consistent about the indentation here. I think a good practice is doing it inline when you have singular value, and nesting it when you're performing computations. Additionally, swift has implicit returns for things like this, so your code would be a lot cleaner without the returns here!
Xhether
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.
Just some small stying issues I addressed in the comments, but great work! I'll approve ASAP after you fix those
|
Very small changes:
1. Also to the past scores part with basketball having a different number of periods thus columns for men and women