Finger painting code

Question

I have a very simple view that handles touch events and draws accordingly. It's nothing significant, but it does use a bit more CPU than I would like (35%). Again, it is the bare minimum (<90 lines), but I can't really think of a simpler way to draw using user input. What would you change?

import UIKit

class DrawView: UIView {

    var mainImageView = UIImageView()

    var tempImageView = UIImageView()

    var r = CGFloat(0)
    var g = CGFloat(0)
    var b = CGFloat(0)
    var a = CGFloat(1)

    var lineWidth = CGFloat(5)

    var dot = true
    var lastPoint = CGPoint.zeroPoint

    required init(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        self.backgroundColor = UIColor.clearColor()
        self.multipleTouchEnabled = false
        mainImageView.frame = self.frame
        tempImageView.frame = self.frame
        self.addSubview(mainImageView)
        self.addSubview(tempImageView)
    }

    override func touchesBegan(touches: Set<NSObject>, withEvent event: UIEvent) {
        dot = true
        if let touch = touches.first as? UITouch {
            lastPoint = touch.locationInView(self)
        }
    }

    func drawLineFrom(fromPoint: CGPoint, toPoint: CGPoint) {
        UIGraphicsBeginImageContextWithOptions(self.frame.size, false, 0)
        let context = UIGraphicsGetCurrentContext()
        tempImageView.image?.drawInRect(self.frame)

        CGContextMoveToPoint(context, fromPoint.x, fromPoint.y)
        CGContextAddLineToPoint(context, toPoint.x, toPoint.y)

        CGContextSetLineCap(context, kCGLineCapRound)
        CGContextSetLineWidth(context, lineWidth)
        CGContextSetRGBStrokeColor(context, r, g, b, a)
        CGContextSetBlendMode(context, kCGBlendModeNormal)

        CGContextStrokePath(context)

        tempImageView.image = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()
    }

    override func touchesMoved(touches: Set<NSObject>, withEvent event: UIEvent) {
        dot = false
        if let touch = touches.first as? UITouch {
            let currentPoint = touch.locationInView(self)
            drawLineFrom(lastPoint, toPoint: currentPoint)
            lastPoint = currentPoint
        }
    }

    override func touchesEnded(touches: Set<NSObject>, withEvent event: UIEvent) {
        if dot {
            drawLineFrom(lastPoint, toPoint: lastPoint)
        }

        UIGraphicsBeginImageContextWithOptions(self.frame.size, false, 0)
        mainImageView.image?.drawInRect(self.frame, blendMode: kCGBlendModeNormal, alpha: a)
        tempImageView.image?.drawInRect(self.frame, blendMode: kCGBlendModeNormal, alpha: a)
        mainImageView.image = UIGraphicsGetImageFromCurrentImageContext()
        UIGraphicsEndImageContext()

        tempImageView.image = nil
    }

    func setColor(color: UIColor) {
        let components = CGColorGetComponents(color.CGColor)
        r = components[0]
        g = components[1]
        b = components[2]
    }

    func erase() {
        mainImageView.image = nil
    }

}

Answer 1

The first thing that strikes me as odd about your code is having the four single-character variables to represent color components. I would rather see a single drawColor variable with the UIColor type. Not only does this eliminate the need for the setColor method, but it also more easily lets us pull that color out for numerous reasons (make some other thing the same color as our DrawView's current drawColor, or see if the DrawView's drawColor is the same shade as something else, etc. There are plenty of reasons. Then only internal to the specific necessary methods do we break that color down into its components for the necessary Core Graphics function call.

---

Our code has no sense of organization. We should put our event-handling methods together, rather than separate them by the big drawLine method.

---

Our drawLine method isn't very nicely named. First of all, we can inspect the method to know that it expects points, and the "from" outside of the parenthesis looks and feels very odd. I'd prefer the method be declared something more like this:

drawLine(from fromPoint: CGPoint, to toPoint: CGPoint)

Which means when it's called by an end user, the call looks like this:

drawView.drawLine(from: pointA, to: pointB)

But internally, we refer to the points as the fromPoint and the toPoint.

---

mainImageView.frame = self.frame
tempImageView.frame = self.frame

This bit isn't really going to work. If these two views are subviews of the DrawView and we intend for them to be in the same spot, we need to better.

For a start, their frame property should be set to self's bounds property. For any UIView, the frame describes its position in its superview, so the X and Y will be how far it is from the top left corner. Meanwhile, its bounds property will always have an X and Y of zero. So if I set my subview's frame to be equal to its parent's bounds property, it will be the same size, but it will be positioned to the parent's top left corner--and I think this is what we're actually trying to achieve right?

But even this isn't enough. What happens when self is resized by something? Now our manually sized subviews are no longer the correct size. We need to programmatically add autolayout constraints so that the four edges of these subviews are pinned to its parent view, so they will always be the same size.

---

Ultimately, there's no real reason to use our image view subviews. Using Core Graphics, we can add lines like this directly to any view. This Code Review question shows an example of drawing lines directly on a view. The only reason we need an image view is if we want to display a UIImage. And there's no need to do so here.

Even if we want to be able to generate a UIImage from what the user has drawn on our DrawView, there are means for doing that in iOS (that question is Objective-C, but it can be done in Swift too). So there really is no reason to be using the image views here at all.