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.

Sign up for GitHub

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

Jump to bottom

Minor cleanup to Highlighter types #3003

Merged
merged 5 commits into from
Jan 6, 2018
Merged

Minor cleanup to Highlighter types #3003

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c28880a
Minor cleanup to Highlighter types
jjatie Nov 14, 2017
df3b852
Fixes for PR
jjatie Nov 15, 2017
e66ba14
Merge remote-tracking branch 'upstream/master' into highlight-logic
jjatie Nov 15, 2017
796cb2d
Merge remote-tracking branch 'upstream/master' into highlight-logic
jjatie Jan 6, 2018
fa1297a
Pulled master and updated code style
jjatie Jan 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 32 additions & 46 deletions Source/Charts/Highlight/BarHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,22 @@ open class BarHighlighter: ChartHighlighter
{
open override func getHighlight(x: CGFloat, y: CGFloat) -> Highlight?
{
let high = super.getHighlight(x: x, y: y)

if high == nil
{
return nil
}
guard let barData = (self.chart as? BarChartDataProvider)?.barData,
let high = super.getHighlight(x: x, y: y)
else { return nil }

if let barData = (self.chart as? BarChartDataProvider)?.barData
let pos = getValsForTouch(x: x, y: y)

if let set = barData.getDataSetByIndex(high.dataSetIndex) as? IBarChartDataSet,
set.isStacked
{
let pos = getValsForTouch(x: x, y: y)

if
let set = barData.getDataSetByIndex(high!.dataSetIndex) as? IBarChartDataSet,
set.isStacked
{
return getStackedHighlight(high: high!,
set: set,
xValue: Double(pos.x),
yValue: Double(pos.y))
}

return getStackedHighlight(high: high,
set: set,
xValue: Double(pos.x),
yValue: Double(pos.y))
} else {
return high
}
return nil
}

internal override func getDistance(x1: CGFloat, y1: CGFloat, x2: CGFloat, y2: CGFloat) -> CGFloat
Expand Down Expand Up @@ -75,25 +67,22 @@ open class BarHighlighter: ChartHighlighter
return high
}

if let ranges = entry.ranges,
guard let ranges = entry.ranges,
ranges.count > 0
{
let stackIndex = getClosestStackIndex(ranges: ranges, value: yValue)

let pixel = chart
.getTransformer(forAxis: set.axisDependency)
.pixelForValues(x: high.x, y: ranges[stackIndex].to)

return Highlight(x: entry.x,
y: entry.y,
xPx: pixel.x,
yPx: pixel.y,
dataSetIndex: high.dataSetIndex,
stackIndex: stackIndex,
axis: high.axis)
}

return nil
else { return nil }

let stackIndex = getClosestStackIndex(ranges: ranges, value: yValue)
let pixel = chart
.getTransformer(forAxis: set.axisDependency)
.pixelForValues(x: high.x, y: ranges[stackIndex].to)

return Highlight(x: entry.x,
y: entry.y,
xPx: pixel.x,
yPx: pixel.y,
dataSetIndex: high.dataSetIndex,
stackIndex: stackIndex,
axis: high.axis)
}

/// - returns: The index of the closest value inside the values array / ranges (stacked barchart) to the value given as a parameter.
Expand All @@ -102,14 +91,11 @@ open class BarHighlighter: ChartHighlighter
/// - returns:
@objc open func getClosestStackIndex(ranges: [Range]?, value: Double) -> Int
{
if ranges == nil
{
return 0
}

guard let ranges = ranges else { return 0 }

var stackIndex = 0

for range in ranges!
for range in ranges
{
if range.contains(value)
{
Expand All @@ -121,8 +107,8 @@ open class BarHighlighter: ChartHighlighter
}
}

let length = max(ranges!.count - 1, 0)
let length = max(ranges.count - 1, 0)

return (value > ranges![length].to) ? length : 0
return (value > ranges[length].to) ? length : 0
}
}
45 changes: 14 additions & 31 deletions Source/Charts/Highlight/ChartHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,15 @@ open class ChartHighlighter : NSObject, IHighlighter
/// - returns:
@objc open func getHighlight(xValue xVal: Double, x: CGFloat, y: CGFloat) -> Highlight?
{
guard let chart = chart
else { return nil }
guard let chart = chart else { return nil }

let closestValues = getHighlights(xValue: xVal, x: x, y: y)
if closestValues.isEmpty
{
return nil
}
guard !closestValues.isEmpty else { return nil }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This guard is harder to read than how it was before. I have to think about it for a second when I try to understand this line. It was more straightforward before. It doesn't reduce nesting so we can switch it back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While guard often does reduce nesting, it's purpose is to indicate that

You use a guard statement to require that a condition must be true in order for the code after the guard statement to be executed. Apple.

It seems reasonable to me to expect reading "only continue if closestValues is not empty"


let leftAxisMinDist = getMinimumDistance(closestValues: closestValues, y: y, axis: YAxis.AxisDependency.left)
let rightAxisMinDist = getMinimumDistance(closestValues: closestValues, y: y, axis: YAxis.AxisDependency.right)
let leftAxisMinDist = getMinimumDistance(closestValues: closestValues, y: y, axis: .left)
let rightAxisMinDist = getMinimumDistance(closestValues: closestValues, y: y, axis: .right)

let axis = leftAxisMinDist < rightAxisMinDist ? YAxis.AxisDependency.left : YAxis.AxisDependency.right
let axis = leftAxisMinDist < rightAxisMinDist ? YAxis.AxisDependency.left : .right

let detail = closestSelectionDetailByPixel(closestValues: closestValues, x: x, y: y, axis: axis, minSelectionDistance: chart.maxHighlightDistance)

Expand All @@ -77,21 +73,15 @@ open class ChartHighlighter : NSObject, IHighlighter
{
var vals = [Highlight]()

guard let
data = self.data
else { return vals }
guard let data = self.data else { return vals }

for i in 0 ..< data.dataSetCount
{
guard let dataSet = data.getDataSetByIndex(i)
guard let dataSet = data.getDataSetByIndex(i),
dataSet.isHighlightEnabled // don't include datasets that cannot be highlighted
else { continue }

// don't include datasets that cannot be highlighted
if !dataSet.isHighlightEnabled
{
continue
}


// extract all y-values from all DataSets at the given x-value.
// some datasets (i.e bubble charts) make sense to have multiple values for an x-value. We'll have to find a way to handle that later on. It's more complicated now when x-indices are floating point.

Expand All @@ -114,13 +104,10 @@ open class ChartHighlighter : NSObject, IHighlighter
else { return highlights }

var entries = set.entriesForXValue(xValue)
if entries.count == 0
if entries.count == 0, let closest = set.entryForXValue(xValue, closestToY: .nan, rounding: rounding)
{
// Try to find closest x-value and take all entries for that x-value
if let closest = set.entryForXValue(xValue, closestToY: Double.nan, rounding: rounding)
{
entries = set.entriesForXValue(closest.x)
}
entries = set.entriesForXValue(closest.x)
}

for e in entries
Expand All @@ -146,14 +133,12 @@ open class ChartHighlighter : NSObject, IHighlighter
var distance = minSelectionDistance
var closest: Highlight?

for i in 0 ..< closestValues.count
for high in closestValues
{
let high = closestValues[i]

if axis == nil || high.axis == axis
{
let cDistance = getDistance(x1: x, y1: y, x2: high.xPx, y2: high.yPx)

if cDistance < distance
{
closest = high
Expand All @@ -173,10 +158,8 @@ open class ChartHighlighter : NSObject, IHighlighter
{
var distance = CGFloat.greatestFiniteMagnitude

for i in 0 ..< closestValues.count
for high in closestValues
{
let high = closestValues[i]

if high.axis == axis
{
let tempDistance = abs(getHighlightPos(high: high) - y)
Expand Down
51 changes: 21 additions & 30 deletions Source/Charts/Highlight/CombinedHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,46 +30,37 @@ open class CombinedHighlighter: ChartHighlighter
{
var vals = [Highlight]()

guard let chart = self.chart as? CombinedChartDataProvider
guard let chart = self.chart as? CombinedChartDataProvider,
let dataObjects = chart.combinedData?.allData
else { return vals }

if let dataObjects = chart.combinedData?.allData
for i in 0..<dataObjects.count
{
for i in 0..<dataObjects.count
let dataObject = dataObjects[i]

// in case of BarData, let the BarHighlighter take over
if barHighlighter != nil && dataObject is BarChartData,
let high = barHighlighter?.getHighlight(x: x, y: y)
{
high.dataIndex = i
vals.append(high)
}
else
{
let dataObject = dataObjects[i]

// in case of BarData, let the BarHighlighter take over
if barHighlighter != nil && dataObject is BarChartData
for j in 0..<dataObject.dataSetCount
{
if let high = barHighlighter?.getHighlight(x: x, y: y)
guard let dataSet = dataObject.getDataSetByIndex(j),
dataSet.isHighlightEnabled // don't include datasets that cannot be highlighted
else { continue }

let highs = buildHighlights(dataSet: dataSet, dataSetIndex: j, xValue: xValue, rounding: .closest)

for high in highs
{
high.dataIndex = i
vals.append(high)
}
}
else
{
for j in 0..<dataObject.dataSetCount
{
guard let dataSet = dataObjects[i].getDataSetByIndex(j)
else { continue }

// don't include datasets that cannot be highlighted
if !dataSet.isHighlightEnabled
{
continue
}

let highs = buildHighlights(dataSet: dataSet, dataSetIndex: j, xValue: xValue, rounding: .closest)

for high in highs
{
high.dataIndex = i
vals.append(high)
}
}
}
}
}

Expand Down
38 changes: 16 additions & 22 deletions Source/Charts/Highlight/HorizontalBarHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,22 @@ open class HorizontalBarHighlighter: BarHighlighter
{
open override func getHighlight(x: CGFloat, y: CGFloat) -> Highlight?
{
if let barData = self.chart?.data as? BarChartData
guard let barData = self.chart?.data as? BarChartData else { return nil }
let pos = getValsForTouch(x: y, y: x)

guard let high = getHighlight(xValue: Double(pos.y), x: y, y: x)
else { return nil }

if let set = barData.getDataSetByIndex(high.dataSetIndex) as? IBarChartDataSet,
set.isStacked
{
let pos = getValsForTouch(x: y, y: x)

guard let high = getHighlight(xValue: Double(pos.y), x: y, y: x)
else { return nil }

if let set = barData.getDataSetByIndex(high.dataSetIndex) as? IBarChartDataSet,
set.isStacked
{
return getStackedHighlight(high: high,
set: set,
xValue: Double(pos.y),
yValue: Double(pos.x))
}

return high
return getStackedHighlight(high: high,
set: set,
xValue: Double(pos.y),
yValue: Double(pos.x))
}
return nil

return high
}

internal override func buildHighlights(
Expand All @@ -50,13 +47,10 @@ open class HorizontalBarHighlighter: BarHighlighter
else { return highlights }

var entries = set.entriesForXValue(xValue)
if entries.count == 0
if entries.count == 0, let closest = set.entryForXValue(xValue, closestToY: .nan, rounding: rounding)
{
// Try to find closest x-value and take all entries for that x-value
if let closest = set.entryForXValue(xValue, closestToY: Double.nan, rounding: rounding)
{
entries = set.entriesForXValue(closest.x)
}
entries = set.entriesForXValue(closest.x)
}

for e in entries
Expand Down
8 changes: 3 additions & 5 deletions Source/Charts/Highlight/PieHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@ open class PieHighlighter: PieRadarHighlighter
{
open override func closestHighlight(index: Int, x: CGFloat, y: CGFloat) -> Highlight?
{
guard let set = chart?.data?.dataSets[0]
guard let set = chart?.data?.dataSets[0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combining these guard statements makes it harder to read. I think it was easier to understand before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about all the other places in the framework where we are writing guard statements like this? Although usually it's written like

guard
    let set = chart?.data?.dataSets[0],
    let entry = set.entryForIndex(index)
    else { return nil }

let entry = set.entryForIndex(index)
else { return nil }

guard let entry = set.entryForIndex(index)
else { return nil }


return Highlight(x: Double(index), y: entry.y, xPx: x, yPx: y, dataSetIndex: 0, axis: set.axisDependency)
}
}
38 changes: 18 additions & 20 deletions Source/Charts/Highlight/PieRadarHighlighter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,32 +23,30 @@ open class PieRadarHighlighter: ChartHighlighter
let touchDistanceToCenter = chart.distanceToCenter(x: x, y: y)

// check if a slice was touched
if touchDistanceToCenter > chart.radius
{
guard touchDistanceToCenter <= chart.radius else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

guard or if here doesn't really make a difference.

Copy link
Collaborator Author

@jjatie jjatie Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You use a guard statement to require that a condition must be true in order for the code after the guard statement to be executed.

So in this case the guard makes it extra clear that the method can return early here

// if no slice was touched, highlight nothing
return nil
}

var angle = chart.angleForPoint(x: x ,y: y)

if chart is PieChartView
{
angle /= CGFloat(chart.chartAnimator.phaseY)
}

let index = chart.indexForAngle(angle)

// check if the index could be found
if index < 0 || index >= chart.data?.maxEntryCountSet?.entryCount ?? 0
{
return nil
}
else
{
var angle = chart.angleForPoint(x: x ,y: y)

if chart is PieChartView
{
angle /= CGFloat(chart.chartAnimator.phaseY)
}

let index = chart.indexForAngle(angle)

// check if the index could be found
if index < 0 || index >= chart.data?.maxEntryCountSet?.entryCount ?? 0
{
return nil
}
else
{
return closestHighlight(index: index, x: x, y: y)
}
return closestHighlight(index: index, x: x, y: y)
}

}

/// - returns: The closest Highlight object of the given objects based on the touch position inside the chart.
Expand Down
Loading