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

unwrap optionals #2698

Merged
merged 7 commits into from
Nov 15, 2017
Merged

unwrap optionals #2698

merged 7 commits into from
Nov 15, 2017

Conversation

russellbstephens
Copy link
Contributor

@russellbstephens russellbstephens commented Aug 9, 2017

an attempt to reduce the number of force unwraps

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #2698 into master will increase coverage by 0.05%.
The diff coverage is 9.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2698      +/-   ##
==========================================
+ Coverage   19.64%   19.69%   +0.05%     
==========================================
  Files         113      113              
  Lines       13792    13720      -72     
==========================================
- Hits         2709     2702       -7     
+ Misses      11083    11018      -65
Impacted Files Coverage Δ
Source/Charts/Charts/RadarChartView.swift 0% <0%> (ø) ⬆️
Source/Charts/Components/MarkerImage.swift 0% <0%> (ø) ⬆️
Source/Charts/Animation/Animator.swift 1.44% <0%> (+0.15%) ⬆️
Source/Charts/Charts/PieRadarChartViewBase.swift 0% <0%> (ø) ⬆️
Source/Charts/Charts/CombinedChartView.swift 0% <0%> (ø) ⬆️
...ata/Implementations/Standard/BubbleChartData.swift 0% <0%> (ø) ⬆️
...ta/Implementations/Standard/ScatterChartData.swift 0% <0%> (ø) ⬆️
...s/Data/Implementations/Standard/BarChartData.swift 3.7% <0%> (+0.06%) ⬆️
...ta/Implementations/Standard/LineChartDataSet.swift 24.19% <0%> (+1.8%) ⬆️
...a/Implementations/Standard/BarChartDataEntry.swift 2.3% <0%> (ø) ⬆️
... and 5 more

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 98552bb...83ec416. Read the comment docs.

@liuxuan30
Copy link
Member

Mind to give some details and what this for?

@russellbstephens
Copy link
Contributor Author

I was just looking through the repo and noticed some areas that could be refactored to remove !. Overall it adds a more swifty feel to your codebase

@pmairoldi
Copy link
Collaborator

Thanks for the submission. Moving to a more swift style is great. I've wanted to do that myself but hadn't had the time. I'd like to merge this change after swift 4 support is merge. Should be soon.

{
foundScrollView = nil
}

var scrollViewPanGestureRecognizer: NSUIGestureRecognizer!

if foundScrollView !== nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

replace the loop with

        let scrollViewPanGestureRecognizer = foundScrollView?.nsuiGestureRecognizers?.first {
            $0 is NSUIPanGestureRecognizer
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not confident in using .first here to achieve the same effect

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not? The code you've replaced the for loop with is not the same. In the for loop, we escape after we find the first NSUIPanGestureRecognizer, in your code it will continue to loop through and find more.

Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

Rather than leave it the old way (using our own implementation of first) why not simply use the built-in method and replace 5 lines with 2 more explicit lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -562,12 +561,12 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
{
if h == nil
{
delegate!.chartValueNothingSelected?(self)
delegate?.chartValueNothingSelected?(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why leave delegate != nil in the if statement if you're going to use optional chaining?
Maybe
if callDelegate, let delegate = delegate { ... } instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -562,12 +561,12 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate
{
if h == nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

To follow the spirit of your PR, you should flip the order of the if-else, i.e.

if let h = h { ... }
else { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jjatie
Copy link
Collaborator

jjatie commented Sep 10, 2017

@russellbstephens I'd encourage you to look at PR #2737. It takes this a lot further and reduces many algorithms to one-liners. Perhaps you could bring some of those changes into here, as currently I can't get that PR to build.

@russellbstephens
Copy link
Contributor Author

@jjatie sure no problem, I put this up to see if you all would be interested in cleaning up the code a bit. Now that I know there is interest, I would be happy to go further with this

@russellbstephens
Copy link
Contributor Author

@jjatie after looking at #2737 I feel you should be able to resolve the conflicts and get it into running order, as these two refactors are separate with different goals

@russellbstephens
Copy link
Contributor Author

@petester42 is this ready to go in?

@liuxuan30
Copy link
Member

liuxuan30 commented Oct 8, 2017

sorry to jump in. Is this on swift 4 or need another convert on your side?

@@ -888,7 +887,7 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate

do
{
try imageData.write(to: URL(fileURLWithPath: path), options: .atomic)
try imageData?.write(to: URL(fileURLWithPath: path), options: .atomic)
Copy link
Member

Choose a reason for hiding this comment

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

Optional chaining can cover up the exception if the image type is not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -120,7 +120,7 @@ open class CombinedChartView: BarLineChartViewBase, CombinedChartDataProvider
{
return nil
}
return (_data as! CombinedChartData!).lineData
Copy link
Member

Choose a reason for hiding this comment

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

It has been sometime we review this part. But seems CombinedChartData! can be CombinedChartData now,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -120,7 +120,7 @@ open class CombinedChartView: BarLineChartViewBase, CombinedChartDataProvider
{
return nil
}
return (_data as! CombinedChartData!).lineData
return (_data as? CombinedChartData)?.lineData
Copy link
Member

Choose a reason for hiding this comment

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

shall we use (_data as! CombinedChartData) or (_data as? CombinedChartData)? ? seems as! is safe here though as? is safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to avoid using !, since in a case where _data === nil ! will crash

@@ -55,7 +55,7 @@ open class CombinedChartView: BarLineChartViewBase, CombinedChartDataProvider

self.highlighter = CombinedHighlighter(chart: self, barDataProvider: self)

(renderer as! CombinedChartRenderer?)!.createRenderers()
(renderer as? CombinedChartRenderer)?.createRenderers()
Copy link
Member

Choose a reason for hiding this comment

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

old code is a bit weird (maybe the compiler bug). But if it's safe unwrap here, why not use (renderer as! CombinedChartRenderer).createRenderers()? renderer has been setup as combined chart renderer in initialize() above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not rely on the assumption renderer !== nil

Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

I agree with @russellbstephens because while the three of us may know it's safe to force unwrap here, it is in no way clear that this is the case. I think a future ticket should involve making renderer a required property since you can't really do anything with a chart without a renderer.

@@ -104,16 +104,16 @@ open class PieChartView: PieRadarChartViewBase
let optionalContext = NSUIGraphicsGetCurrentContext()
guard let context = optionalContext else { return }

renderer!.drawData(context: context)
renderer?.drawData(context: context)
Copy link
Member

Choose a reason for hiding this comment

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

why ? here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its best to avoid ! in swift

let touchLocation = touch?.location(in: self)

processRotationGestureBegan(location: touchLocation!)
if let touch = touches.first {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why use if here. Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -102,17 +102,17 @@ open class BarChartDataEntry: ChartDataEntry

open func sumBelow(stackIndex :Int) -> Double
{
if _yVals == nil
guard let _yVals = _yVals else
Copy link
Member

Choose a reason for hiding this comment

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

shall we keep using _ for local instant here? seems a little bit weird, though we don't have to change other code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@russellbstephens
Copy link
Contributor Author

is there any hope of this going in?

@@ -115,9 +103,9 @@ open class Animator: NSObject
elapsed = duration
}

if _easingX != nil
if let _easingX = _easingX
Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

Why isn't this one line?
phaseX = _easingX?(elapsed, duration) ?? Double(elapsed / duration)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
phaseY = _easingY!(elapsed, duration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be one line as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -233,7 +233,7 @@ open class BarLineChartViewBase: ChartViewBase, BarLineScatterCandleBubbleChartD

context.restoreGState()

renderer!.drawExtras(context: context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use
guard let data = data, let renderer = renderer else { return }
at the beginning of the function?

Logically, a draw() function would require a renderer to draw anything, and looking through the implementation of the function this is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -533,7 +533,7 @@ open class BarLineChartViewBase: ChartViewBase, BarLineScatterCandleBubbleChartD

let h = getHighlightByTouchPoint(recognizer.location(in: self))

if h === nil || h!.isEqual(self.lastHighlighted)
if h === nil || h?.isEqual(self.lastHighlighted) ?? false
Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

this can be just if h?.isEqual(self.lastHighlighted) ?? true { because we want to enter the if statement if h == nil

Copy link
Collaborator

Choose a reason for hiding this comment

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

The former force unwrapping was also acceptable, because if the first condition passed on the or statement, we would have already entered the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -753,7 +753,7 @@ open class BarLineChartViewBase: ChartViewBase, BarLineScatterCandleBubbleChartD

if ((h === nil && lastHighlighted !== nil) ||
(h !== nil && lastHighlighted === nil) ||
(h !== nil && lastHighlighted !== nil && !h!.isEqual(lastHighlighted)))
(h !== nil && lastHighlighted !== nil && !(h?.isEqual(lastHighlighted) ?? false)))
Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

Here we've already checked that h != nil in the AND statement. If h == nil escape early from the AND statement, so it is safe to force unwrap h

Alternatively, we can do the same thing as before and write
lastHighlighted !== nil && !(h?.isEqual(lastHighlighted) ?? true)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently there is an issue in the last statement. If the whole expression was evaluated, then in the last component (!(h?.isEqual(lastHighlighted) ?? false)) you would return true when h == nil which is not what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -931,7 +931,7 @@ open class BarLineChartViewBase: ChartViewBase, BarLineScatterCandleBubbleChartD
))
{
var scrollView = self.superview
while (scrollView !== nil && !scrollView!.isKind(of: NSUIScrollView.self))
while (scrollView !== nil && !(scrollView?.isKind(of: NSUIScrollView.self) ?? false))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue here where if the last component is evaluated and scrollView == nil we return true
Easiest way to solve this is to replace
(scrollView !== nil && !(scrollView?.isKind(of: NSUIScrollView.self) ?? false))
with
!(scrollView?.isKind(of: NSUIScrollView.self) ?? true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// if no position specified, draw on default position
if position == nil
{
let position: CGPoint
Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

Could be cleaner to do this.

 let p = description.position ?? CGPoint(x: bounds.width - _viewPortHandler.offsetRight - description.xOffset,
                                            y: bounds.height - _viewPortHandler.offsetBottom - description.yOffset - description.font.lineHeight)

Might just come down to preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

let position: CGPoint
if let descriptionPosition = description.position {
position = descriptionPosition
} else {
let frame = self.bounds
Copy link
Collaborator

@jjatie jjatie Nov 6, 2017

Choose a reason for hiding this comment

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

It's generally good practice to short a variable call (i.e. let width = self.frame.size.width), but in this case it is unnecessary and wrong. Unnecessary because you don't need to explicitly state self, and wrong because in Cocoa frame and bounds mean two different things. frame should have never been here in the first place.

So we can just write

CGPoint(x: bounds.width - _viewPortHandler.offsetRight - description.xOffset,
        y: bounds.height - _viewPortHandler.offsetBottom - description.yOffset - description.font.lineHeight)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

let touchLocation = touch?.location(in: self)

processRotationGestureMoved(location: touchLocation!)
if let touch = touches.first {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the if let touch = touches.first into if rotationEnabled && !rotationWithTwoFingers
i.e.

if rotationEnabled && !rotationWithTwoFingers, let touch = touches.first { ... }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we don't need to check to see if touches.first is non-nil. We know there is at least one touch because the delegate method is called. Even Apple force unwraps this their sample code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

let touchLocation = touch?.location(in: self)

processRotationGestureEnded(location: touchLocation!)
if let touch = touches.first {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -118,19 +118,19 @@ open class RadarChartView: PieRadarChartViewBase

if drawWeb
{
renderer!.drawExtras(context: context)
renderer?.drawExtras(context: context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

guard let data = _data, let renderer = renderer else { return } at the beginning of the function,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -102,7 +102,7 @@ open class MarkerImage: NSObject, IMarker
height: size.height)

NSUIGraphicsPushContext(context)
image!.draw(in: rect)
image?.draw(in: rect)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like there should be a guard let image = image else { return } at the beginning of the method. Drawing will do nothing if image == nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -36,18 +36,18 @@ open class MarkerView: NSUIView, IMarker
{
offset.x = -point.x
}
else if chart != nil && point.x + width + offset.x > chart!.bounds.size.width
else if let chart = chart, point.x + width + offset.x > chart.bounds.size.width
Copy link
Collaborator

Choose a reason for hiding this comment

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

guard let chart = chartView else { return } at the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this func must return a CGPoint

Copy link
Collaborator

Choose a reason for hiding this comment

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

guard let chart = chartView else { return offset }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -102,17 +102,17 @@ open class BarChartDataEntry: ChartDataEntry

@objc open func sumBelow(stackIndex :Int) -> Double
{
if _yVals == nil
guard let yVals = _yVals else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throughout the project we're being inconsistent in how we write closures. Sometimes we open the closure on a new line, sometimes not, and sometimes we write one liners. Since you're going through a significant portion of the project already, can we consolidate on a format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love too, personally I think opening on the same line is fine

@@ -27,8 +27,7 @@ open class BubbleChartData: BarLineScatterCandleBubbleChartData
/// Sets the width of the circle that surrounds the bubble when highlighted for all DataSet objects this data object contains
@objc open func setHighlightCircleWidth(_ width: CGFloat)
{
for set in (_dataSets as? [IBubbleChartDataSet])!
{
(_dataSets as? [IBubbleChartDataSet])?.forEach { set in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be a one liner as
(_dataSets as? [IBubbleChartDataSet])?.forEach { $0.highlightCircleWidth = width }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@russellbstephens
Copy link
Contributor Author

@jjatie how's it looking?

@jjatie
Copy link
Collaborator

jjatie commented Nov 6, 2017

Looks fine to me!

@russellbstephens
Copy link
Contributor Author

nice!

@russellbstephens
Copy link
Contributor Author

so merge?

@russellbstephens
Copy link
Contributor Author

:)

@jjatie
Copy link
Collaborator

jjatie commented Nov 9, 2017

@liuxuan30

@liuxuan30
Copy link
Member

seems I don't have further questions. @danielgindi @petester42, any comments?

@pmairoldi
Copy link
Collaborator

The review that has been done by everyone seems adequate to me.

@liuxuan30
Copy link
Member

ok, merging.

@liuxuan30 liuxuan30 merged commit fc22a27 into ChartsOrg:master Nov 15, 2017
@russellbstephens russellbstephens deleted the cleanup branch November 19, 2017 14:54
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.

5 participants