Polygon and Polyline addition

[odmir]

I was thinking about what @karwa said in PR #64 regarding returning early instead of crashing in drawing methods of Renderer:

Hmm... I'm not sure if we should just silently return without drawing anything if the number of points are invalid. It might be worth making these in to precondition(...) checks, which crash on invalid input.

In general it's not a good idea to silently return without doing anything - they can be difficult to debug or mask other errors. We told the renderer to draw, so it should draw (or fail loudly, telling me why it couldn't draw).

So I thought that instead of checking for the number of points passed to the drawPlotLines and drawSolidPolygon methods in each possible and future renderer, and hoping nobody forgets to do it, or doing the checks in earlier parts of the program (possibly leading to sprinkled and redundant .count checks), we could maybe do something differently.

The idea is to replace [Point] in these methods with two new structs: Polyline and Polygon.
Their purpose is to better utilize the type system so the information that these types must have at least 2 or 3 points can be propagated and the compiler helps us along the way. This pushes the responsibility of checking the amount of points as early as possible if it really is needed. If we don't check, we can't really construct any of these structs and ask the renderers to draw them.

In the case of the drawData method of LineChart for example, in order to ask to draw anything we must first construct a Polyline and for that we must give it at least 2 points. But in this case we could eventually push Polyline up a level and make the code that generates scaledValues take care of checking that there are at least 2 values in there by using Polyline as the type for scaledValues, and so on.

I think this is a good approach taking leverage of Swift's type system to propagate information and the compiler to enforce we do the correct thing.

I'd like to hear your thoughts!

Ps: I also took the opportunity to change the drawPlotLines method on the QuartzRenderer to draw the the lines correctly instead of calling drawLine for each pair of points.

Edit: Correct mistakes and change order of Polyline and Polygon above to match "2 or 3 points" later on.

[karwa]

I need a bit more time to look at the implementation, but very much +1 to the idea! 👍

[karwa]

I'm in favour of the idea of adding these geometry primitive types, but I think they should both just wrap an Array<Point> - as you can see, most of the time you're just extracting the first few elements from an existing array via a subscript and copying the rest in to the tail. We can just wrap that array directly and build the count-check in to the initialiser.

[karwa]

Is this really necessary? We can also get a guarantee that there are at least 3 points with a single array and this initialiser:

public init?(points: [Point]) {
       guard points.count >= 3 else { return nil }
       self.init(points)
}

We could also add a precondition check on the polygon's didSet, checking that the number of values is not less than 3.

This would also make it trivial to turn a Polygon in to a Polyline (e.g. if you wanted to draw a stroke pattern on the polygon shape), since they would both just wrap Array<Point>

[odmir]

By the polygon's didSet do you mean the wrapped array's didSet? The idea was to force the programmer to always give at least 2/3 points at the point of creation so no check is needed or optionally using the failable initializer where the check happens once at runtime, but I agree that this is probably too much for what is essentially a wrapper.

[odmir]

@karwa may I draw your attention to this? I will implement it as an array wrapper. Assuming you meant the didSet of the internal array of points, do we intend to allow people to just modify the internal array directly or should it be readonly? Maybe it makes sense to allow that, so these structs can start to be used earlier and points can be appended and such, but checking for the precondition for every single operation on the array may start hurting performance significantly eventually.

[karwa]

Sure - I just mentioned the didSet because it is currently a public var, which allows mutations after the initialiser. Performance isn't really a massive concern - actually rendering the image will be the vast majority of the work. If it becomes a problem, we could make it @inlinable so the compiler can optimise those checks away. But really, only if it's a problem.

[odmir]

I've solved the merge conflicts and made the discussed changes. One last review and its good to merge! @karwa @KarthikRIyer

[karwa]

Looks good - just a few minor issues.

[odmir]

I made the requested changes and I think everything is ready to merge!
@KarthikRIyer @karwa

[karwa]

Looks good to me! 👍

[KarthikRIyer]

Sorry for the delay in taking a look at this @odmir @karwa .
This looks awesome! Merging.