Creating an area graph in iOS

I am trying to create an area graph from the upper and lower limit values.

First I created two line graph from those points as:

Sample data points are:

var line1dataPoints:[CGPoint] = [CGPoint(x: 10, y: 120),CGPoint(x: 20, y: 130),CGPoint(x: 20, y: 120),CGPoint(x: 70, y: 130),CGPoint(x: 70, y: 120),CGPoint(x: 80, y: 130),CGPoint(x: 90, y: 120),CGPoint(x: 100, y: 110),CGPoint(x: 120, y: 120),CGPoint(x: 120, y: 130)]
 var line2dataPoints:[CGPoint] =  [CGPoint(x: 10, y: 170),CGPoint(x: 20, y: 180),CGPoint(x: 20, y: 170),CGPoint(x: 70, y: 180),CGPoint(x: 70, y: 170),CGPoint(x: 80, y: 180),CGPoint(x: 90, y: 170),CGPoint(x: 100, y: 180),CGPoint(x: 120, y: 160),CGPoint(x: 120, y: 170)]

Method to create those line graphs are:

func addFirstLine(){

        let graphPath = UIBezierPath()
        graphPath.moveToPoint(line1dataPoints[0])

        for i in 1..<line1dataPoints.count {

            let nextPoint = line1dataPoints[i]
            graphPath.addLineToPoint(nextPoint)

        }
        let lineLayer = CAShapeLayer()
        lineLayer.lineWidth = 1.0
        lineLayer.path = graphPath.CGPath
        lineLayer.strokeColor = UIColor.blueColor().CGColor
        lineLayer.fillColor = UIColor.clearColor().CGColor
        self.view.layer.addSublayer(lineLayer)

    }

    func addSecondLine(){

        let graphPath = UIBezierPath()
        graphPath.moveToPoint(line2dataPoints[0])

        for i in 1..<line2dataPoints.count {

            let nextPoint = line2dataPoints[i]
            graphPath.addLineToPoint(nextPoint)

        }
        let lineLayer = CAShapeLayer()
        lineLayer.lineWidth = 1.0
        lineLayer.path = graphPath.CGPath
        lineLayer.strokeColor = UIColor.redColor().CGColor
        lineLayer.fillColor = UIColor.clearColor().CGColor
        self.view.layer.addSublayer(lineLayer)
    }

I created an area graph with the method as:

```
func createAreaGraph(){

let rev = Array(line2dataPoints.reverse())
line1dataPoints.a

To me, this seems like a perfectly good way to create the area graph! I think you've done something that's simple, straightforward, and easy to understand. Nice work!

Don't Repeat Yourself

The one thing that I think could be improved is the drawing of the line graphs. You have 2 functions which look identical except one is using line1dataPoints and the other is using line2dataPoints as its input. If you ever decide that something needs to change in the drawing or you need to add or remove anything, you now have to do it in 2 places. Why not just have a single function and pass in the array you want to use? Something like this:

func drawLineGraph(let dataPoints: [CGPoint]) {

    let graphPath = UIBezierPath()
    graphPath.moveToPoint(dataPoints[0])

    for i in 1..<dataPoints.count {

        let nextPoint = dataPoints[i]
        graphPath.addLineToPoint(nextPoint)

    }

    let lineLayer = CAShapeLayer()
    lineLayer.lineWidth = 1.0
    lineLayer.path = graphPath.CGPath
    lineLayer.strokeColor = UIColor.redColor().CGColor
    lineLayer.fillColor = UIColor.clearColor().CGColor
    self.view.layer.addSublayer(lineLayer)
}

Naming

You probably noticed that I renamed the function above to something more descriptive. Your original name was kind of vague. addFirstLine() leaves me wondering what you're adding it to?

But you are actually adding a layer to something at the end of the function. That's kind of surprising! Your original name implied a line was being added to... something. But really what's being added is a layer! And this is a side effect of the function!

Avoid Side Effects

If you want to add a layer to another layer, you should make that explicit in the function's name and definition. You should be either returning the layer that needs to be added, or passing in the layer to add the new layer to. As it is, this function creates a drawing path, then creates a layer to draw it into, then adds it to another layer. But the function definition looks like it just examines some data points. A better function definition might look something like this:

func addLineLayer(let dataPoints: [CGPoint], to layer: CALayer)

Now when you call it, you'll write:

addLineLayer(line1DataPoints, to:self.view.layer)

and it's much more clear to a reader what's going on. (Personally, I'd rename line1DataPoints to something more descriptive, too. Data points from what? Do they represent some maximum or minimum values?

StackExchange Code Review Q#142585, answer score: 2