Refactors optionTapped func, adds guard to avoid crash. #3329
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3329 +/- ##
=======================================
Coverage 28.32% 28.32%
=======================================
Files 117 117
Lines 15669 15669
Branches 320 320
=======================================
Hits 4438 4438
Misses 11188 11188
Partials 43 43Continue to review full report at Codecov.
|
| @@ -142,35 +142,30 @@ class LineChart1ViewController: DemoBaseViewController { | |||
| } | |||
|
|
|||
| override func optionTapped(_ option: Option) { | |||
| guard let sets = chartView.data?.dataSets as? [LineChartDataSet] else { | |||
| for set in chartView.data!.dataSets as! [LineChartDataSet] { | ||
| set.drawFilledEnabled = !set.drawFilledEnabled | ||
| } | ||
| sets.forEach{ $0.drawFilledEnabled = !$0.drawFilledEnabled } |
There was a problem hiding this comment.
a space between forEach and {
valeriyvan
changed the title
| } | ||
| } | ||
| data.dataSets.filter { $0 is LineChartDataSet } | ||
| .forEach { $0.drawValuesEnabled = !$0.drawValuesEnabled } |
There was a problem hiding this comment.
This is fine here, but if you plan on contributing to the Charts framework avoid this. Here we have to loop through dataSets twice to enable toggle drawValuesEnabled, whereas something like :
for case let set as LineChartDataSet in data.dataSets {
set.drawValuesEnabled = !set.drawValuesEnabled
}
only has to loop once. In the framework, performance matters.
I am curious what the performance characteristics of:
data.dataSets.lazy
.flter { ... }
.forEach { ... }
are like.
There was a problem hiding this comment.
I don't know an answer for your last question.
Definitely, sequence.filter{ $0.condition }.forEach{ $0.doSomething } is slower than sequence.forEach{ if $0.condition { $0.doSomething } }. One is faster, another is readable. I guess, here, where we loop over data sets, minor performance loss could be sacrificed for readability.
valeriyvan
changed the title
|
done |
|
done done |
|
@valeriyvan take a look at my comment on #3327 |
|
After putting so much effort into fixing bug the way you suggested, you change your mind( |
|
I know it sucks (it happens with my own PRs a lot). Sometimes you just need to see the outcome of a decision before you see it's wrong. |
Solves #3327.
As for me it doesn't solve origin of a problem.
For charts with the only dataset option to toggle dataset should't be shown at all.