The backing var is not necessary. by jjatie · Pull Request #3000 · ChartsOrg/Charts

jjatie · 2017-11-14T01:21:46Z

Appears to have been a way to expose the var to obj-c like so many others in the framework.
Removed duplicated calls to notifyDataSetChanged

Appears to have been a way to expose the var to obj-c like so many others in the framework.

codecov-io · 2017-11-14T03:51:17Z

Codecov Report

Merging #3000 into master will increase coverage by <.01%.
The diff coverage is 27.11%.

@@            Coverage Diff             @@
##           master    #3000      +/-   ##
==========================================
+ Coverage   19.45%   19.46%   +<.01%     
==========================================
  Files         113      113              
  Lines       16044    15995      -49     
  Branches      247      248       +1     
==========================================
- Hits         3122     3114       -8     
+ Misses      12884    12845      -39     
+ Partials       38       36       -2

Impacted Files	Coverage Δ
Source/Charts/Charts/RadarChartView.swift	`0% <0%> (ø)`	⬆️
...s/Data/Implementations/Standard/ChartDataSet.swift	`37.08% <27.58%> (+2.69%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 159e0f7...e5478b8. Read the comment docs.

liuxuan30 · 2017-12-09T02:37:30Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

    open override func entryForIndex(_ i: Int) -> ChartDataEntry?
    {
-        guard i >= 0 && i < _values.count else {
+        guard values.indices.contains(i) else {


seems this is O(n) cost, while the old one is O(1)?

@liuxuan30 For future reference, I've looked into this, and all Range types have O(1) complexity. They perform a bounds check internally instead of iterating through the range. So this:

(values.startIndex..<values.endIndex).contains(i)

Performs the same, and, I feel, is more clear.

liuxuan30 · 2017-12-09T02:39:40Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift


        var low = 0
-        var high = _values.count - 1
+        var high = values.endIndex - 1


just keep .count so it's consistent with others

count and endIndex aren't necessarily the same, and endIndex is safer (when that's really want we want). When we are able to properly add Collection conformance to ChartDataSet then it will become possible to call this on a subrange of a ChartDataSet where startIndex isn't guaranteed to be 0 and endIndex isn't guaranteed to be count.

liuxuan30 · 2017-12-09T02:46:33Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift


    @objc public init(values: [ChartDataEntry]?, label: String?)
    {
+        self.values = values ?? []


if [] and [ChartDataEntry]() are both valid, shall we just use one of them?

Just curious, do you know whose performance is better? like [] may need to check the type and call the initializer?

The performance is identical in this case. This is a compile time check, not a runtime check. It just looks cleaner this way.

liuxuan30 · 2017-12-09T02:48:39Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

            }
        }
-
-        if removed


I don't see this can be deleted?

Because values has a property observer that takes care of this now.

Sorry, did this observer already exist? Or new added? I don't see it.

It already existed.

the last commit seems just reflect the change about count/endIndex. Just point me the place. Weird I cannot see it

It is a setter call. Try running this in a playground, you will see that the print statement is called.

var x = [1, 2, 3, 4] { didSet { print("ITEM REMOVED") } } x.remove(at: 1)

what... that changed my understanding.
I will check later, and if so, merging soon.

Is this behavior documented? I walked through https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/Properties.html, not mentioning this specifically.

Indirectly, yes.

If you pass a property that has observers to a function as an in-out parameter, the willSet and didSet observers are always called. This is because of the copy-in copy-out memory model for in-out parameters: The value is always written back to the property at the end of the function

Modification of Array<Element> are all in-out (otherwise every time you modified an array, new memory would need to be allocated).

Jesus, thanks. All good

liuxuan30 · 2017-12-09T02:49:30Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

-
-        let removed = entry != nil
-
-        if removed


same as line 474, why delete here?

same as line 474

liuxuan30 · 2017-12-09T02:49:43Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

-
-        let removed = entry != nil
-
-        if removed


same as line 474

…lues-backing-var

liuxuan30 · 2017-12-11T02:01:10Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift


    @objc public init(values: [ChartDataEntry]?, label: String?)
    {
+        self.values = values ?? []


Just curious, do you know whose performance is better? like [] may need to check the type and call the initializer?

liuxuan30 · 2017-12-11T02:11:28Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

-        var low = 0
-        var high = _values.count - 1
+        var low = values.startIndex
+        var high = values.endIndex - 1


When we are able to properly add Collection conformance to ChartDataSet then it will become possible to call this on a subrange of a ChartDataSet where startIndex isn't guaranteed to be 0 and endIndex isn't guaranteed to be count.

Got it. But if a sub set of ChartDataSet is called upon, I think the startIndex and endIndex is still from 0 to count, since we are on a sub set?

Depends on how the collection is implemented. If slicing is implemented, then no. startIndex and endIndex are safer and they work the same as 0 and count for standard collection implementations, but still keep it safe for more sophisticated implementations.

liuxuan30 · 2017-12-11T02:11:41Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

    {
        var low = 0
-        var high = _values.count - 1
+        var high = values.count - 1


do you mean we shall change all .count into .endIndex in the future after collection adoption? Or just some.

We should use count when we actually want to know how many of something there is, and endIndex when we mean the end of the collection. I'll double check, but assuming this is like the other method implementation, this should be endIndex.

I'll review the PR again to make sure I'm consistent.

Sure, I don't mean you need to change others. Just to clarify what we should use. I checked the method names and they don't reflect clearly count or boundary. Sometimes we don't clearly know which to use, since we get used to using count to reflect the boundary.

Are we good to go on this one then?

just one about the observer. I can't see it?

liuxuan30 · 2017-12-11T02:15:20Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

            }
        }
-
-        if removed


Sorry, did this observer already exist? Or new added? I don't see it.

…lues-backing-var

where appropriate

liuxuan30 · 2017-12-11T02:43:15Z

oops, just made some experiments on the "project" tab.
@jjatie are you able to edit or add new items in the project I created "More Swift"? It may be a better place to manage these PRs

jjatie · 2017-12-11T02:45:01Z

I am not able to add/edit anything to the project

liuxuan30 · 2017-12-11T02:49:16Z

eh.. Do you like to join in? If so we can ping Daniel to set it up. gitter maybe faster

jjatie · 2017-12-11T02:49:49Z

I'd be happy to

liuxuan30 · 2017-12-13T02:32:23Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

-
+
    /// Use this method to tell the data set that the underlying data has changed
    open override func notifyDataSetChanged()


It seems we can safely remove open override func notifyDataSetChanged()? Since its superclass ChartBaseDataSet.notifyDataSetChanged() already called calcMinMax()

You are correct! I just tested to verify.

what you think? It's ok either keep or remove.

I'm removing it now

Just find a typo ifnot -> if not

…r' into dataset-remove-values-backing-var

@objc

* 'master' of https://github.com/danielgindi/Charts: (23 commits) Update ViewPortHandler.swift (ChartsOrg#3143) add option to build demo projects unit tests on iOS (ChartsOrg#3121) Replaced relevant `ChartUtils` methods with `Double` extensions (ChartsOrg#2994) Update 4.0.0 with master (ChartsOrg#3135) Removed redundant ivars in BarLineChartViewBase (ChartsOrg#3043) fix ChartsOrg#1830. credit from ChartsOrg#2049 (ChartsOrg#2874) Makes ChartsDemo compiling again (ChartsOrg#3117) Fixed using wrong axis (Issue ChartsOrg#2257) Removed methods and properties deprecated in 1.0 (ChartsOrg#2996) for ChartsOrg#3061 revert animationUpdate() and animationEnd() not trigger crash if subclass does nothing The backing var is not necessary. (ChartsOrg#3000) Replaced `ChartUtils.Math` in favour of an extension on `FloatingPoint` (ChartsOrg#2993) Minor logic cleanup (ChartsOrg#3041) Fix a bug may cause infinite loop. (ChartsOrg#3073) for ChartsOrg#2745. chart should be weak. fileprivate -> private (ChartsOrg#3042) Removed `isKind(of:)` Removed @objc from internal properties Fixes for PR Made use of `==` where appropriate to simplify logic ... # Conflicts: # Source/Charts/Data/Implementations/Standard/LineChartDataSet.swift # Source/Charts/Renderers/BarChartRenderer.swift

* 'master' of https://github.com/danielgindi/Charts: (34 commits) Fixed X-Axis Labels Not Showing (ChartsOrg#3154) (ChartsOrg#3174) fix programatical unhighlighting for BarCharView (ChartsOrg#3159) Give the users customizable axis label limits (Fixes ChartsOrg#2085) (ChartsOrg#2894) bump pod version chart views now use open legend renderer property instead of internal one (ChartsOrg#3149) Fix axis label disappear when zooming in deep enough (ChartsOrg#3132) added DataApproximator+N extension (ChartsOrg#2848) Minor cleanup to Highlighter types (ChartsOrg#3003) Refactored ChartUtils method into CGPoint extension (ChartsOrg#3087) Update ViewPortHandler.swift (ChartsOrg#3143) add option to build demo projects unit tests on iOS (ChartsOrg#3121) Replaced relevant `ChartUtils` methods with `Double` extensions (ChartsOrg#2994) Update 4.0.0 with master (ChartsOrg#3135) Removed redundant ivars in BarLineChartViewBase (ChartsOrg#3043) fix ChartsOrg#1830. credit from ChartsOrg#2049 (ChartsOrg#2874) Makes ChartsDemo compiling again (ChartsOrg#3117) Fixed using wrong axis (Issue ChartsOrg#2257) Removed methods and properties deprecated in 1.0 (ChartsOrg#2996) for ChartsOrg#3061 revert animationUpdate() and animationEnd() not trigger crash if subclass does nothing The backing var is not necessary. (ChartsOrg#3000) ... # Conflicts: # Source/Charts/Data/Implementations/Standard/LineChartDataSet.swift # Source/Charts/Highlight/BarHighlighter.swift # Source/Charts/Renderers/BarChartRenderer.swift

The backing far is not necessary.

fbe84fd

Appears to have been a way to expose the var to obj-c like so many others in the framework.

liuxuan30 requested changes Dec 9, 2017

View reviewed changes

jjatie added 3 commits December 9, 2017 15:20

Corrections for PR

048089b

Merge remote-tracking branch 'upstream/master' into dataset-remove-va…

b0ac5dc

…lues-backing-var

Pulled master

cefe539

liuxuan30 reviewed Dec 11, 2017

View reviewed changes

jjatie added 2 commits December 10, 2017 22:28

Merge remote-tracking branch 'upstream/master' into dataset-remove-va…

d85ea7e

…lues-backing-var

count -> endIndex

23be3e5

where appropriate

liuxuan30 reviewed Dec 13, 2017

View reviewed changes

liuxuan30 and others added 3 commits December 13, 2017 10:35

fix a typo ifnot -> if not

2b3dad2

Just find a typo ifnot -> if not

Removed redundant notifyDataSetChanged()

54b2a88

Merge remote-tracking branch 'origin/dataset-remove-values-backing-va…

e5478b8

…r' into dataset-remove-values-backing-var

liuxuan30 approved these changes Dec 14, 2017

View reviewed changes

liuxuan30 merged commit d4a3d4f into ChartsOrg:master Dec 14, 2017

jjatie deleted the dataset-remove-values-backing-var branch December 14, 2017 01:47

liuxuan30 mentioned this pull request Jan 30, 2018

Restored old performance in ChartDataSet #3216

Merged



		/// Use this method to tell the data set that the underlying data has changed
		open override func notifyDataSetChanged()

Uh oh!

Conversation

jjatie commented Nov 14, 2017

Uh oh!

codecov-io commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxuan30 Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxuan30 Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjatie Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxuan30 Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxuan30 commented Dec 11, 2017

Uh oh!

codecov-io commented Nov 14, 2017 •

edited

Loading

liuxuan30 Dec 11, 2017 •

edited

Loading

liuxuan30 Dec 13, 2017 •

edited

Loading

jjatie Dec 13, 2017 •

edited

Loading

liuxuan30 Dec 11, 2017 •

edited

Loading

liuxuan30 commented Dec 11, 2017 •

edited

Loading

liuxuan30 Dec 13, 2017 •

edited

Loading

jjatie Dec 13, 2017 •

edited

Loading