Skip to content
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

The backing var is not necessary. #3000

Merged
merged 9 commits into from
Dec 14, 2017
Merged

The backing var is not necessary. #3000

merged 9 commits into from
Dec 14, 2017

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Nov 14, 2017

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
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

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

Impacted file tree graph

@@            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.


/// - returns: The entry object found at the given index (not x-value!)
/// - throws: out of bounds
/// if `i` is out of bounds, it may throw an out-of-bounds exception
open override func entryForIndex(_ i: Int) -> ChartDataEntry?
{
guard i >= 0 && i < _values.count else {
guard values.indices.contains(i) else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@@ -228,27 +209,27 @@ open class ChartDataSet: ChartBaseDataSet
var entries = [ChartDataEntry]()

var low = 0
var high = _values.count - 1
var high = values.endIndex - 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just keep .count so it's consistent with others

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

removed = true
break
}
}

if removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this can be deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already existed.

Copy link
Member

@liuxuan30 liuxuan30 Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@liuxuan30 liuxuan30 Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jjatie jjatie Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jesus, thanks. All good


let removed = entry != nil

if removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as line 474, why delete here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as line 474


let removed = entry != nil

if removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as line 474

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as line 474

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var low = 0
var high = _values.count - 1
var low = values.startIndex
var high = values.endIndex - 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -291,15 +272,15 @@ open class ChartDataSet: ChartBaseDataSet
rounding: ChartDataSetRounding) -> Int
{
var low = 0
var high = _values.count - 1
var high = values.count - 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@liuxuan30 liuxuan30 Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we good to go on this one then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

removed = true
break
}
}

if removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liuxuan30
Copy link
Member

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
Copy link
Collaborator Author

jjatie commented Dec 11, 2017

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

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 11, 2017

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

@jjatie
Copy link
Collaborator Author

jjatie commented Dec 11, 2017

I'd be happy to

}
}


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

@liuxuan30 liuxuan30 Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@jjatie jjatie Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct! I just tested to verify.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing it now

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@liuxuan30 liuxuan30 merged commit d4a3d4f into ChartsOrg:master Dec 14, 2017
@jjatie jjatie deleted the dataset-remove-values-backing-var branch December 14, 2017 01:47
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Jan 2, 2018
* '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
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Jan 20, 2018
* '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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants