Erica Sadun

Where technology meets something or other

Swift rewrite challenge

In searching around this morning for some examples of complicated if-then-else-else-else statements on gist (if you have any, please point me to them!), I stumbled across this function for scaling and cropping an image.

I immediately decided to re-write the function in my more personal style, to see what kind of changes I’d introduce and whether any of them could be generalized and discussed as  potential book fodder / post fodder.

Before looking at my version, which is here, you may want to whip up your own take on things and then return to see why I made some of the decisions to introduce changes in my take on it.

My Changes

If you do the math, my version takes about the same number of lines as the original, both are about 38-43 lines of code. Both use Swift 2 code and not Swift 3 code. My code uses more comments, fewer vars, more lets, more conjoined code, and more multi-line calls.

While the original performs a fill-image operation, mine does both fit and fill, using defaulted arguments, which is one of the reasons my function signature occupies 5 lines compared to the original 1-line signature. I find myself more and more breaking signatures down to multiple lines to allow better labeling, defaulting, and documentation.

I’m less worried these days about taking up vertical space as I find that stacked parameter lists are easier to read and understand. Each parameter line may contain an external label, an internal label, a type, and possibly a defaulted value. Swift’s move towards complex parameters deserves the presentation that best exposes these items to their full expression.

Next, I converted the initial if-check to a guard statement. When a function’s code is meant to test initial conditions and immediately leave scope if those conditions are met, it seems to me a guard statement better represents that entry point than “if”.

In calculating the scale factor, I perform a tuple assignment. These factors are semantically parallel, so their assignment is joined and parallelized as well. It’s a fairly Swift-specific approach, and one that should be used sparingly but this fits my parallelism criteria, and I do the same just below when calculating the scaled width and height.

I follow this with a pair of ternary tests, first for choosing whether to use fit or fill, and then choosing the scale factor that applies the fit or fill set by the fitImage Boolean parameter. I think it’s pretty straight forward. A fit operation constrains itself to the smaller of the two scales and a fill to the larger.

In creating the drawing destination, I decided to break out my CGPoint construction to three lines. Again I felt this read better and was easier to follow than placing both the x and y parameters on a single line, which grew quite long. The top-down alignment draws attention to the parallels between x and y and width and height used in the two parameters.

I took some liberties in the UIGraphics drawing section, deciding to use a do clause for the actual drawing. In Objective-C, I often use scoping to distinguish the drawing operations from the bracketing begin and end calls. Because of scoping, I had to pull the scaledImage constant declaration out of the do clause but since Swift now lets me defer the assignment to the constant, this worked out pretty nicely.

In the drawing block, I decided to place the background fill on a single line. I initially had this on two lines, but felt it was drawing too much attention away from the actual operation (drawing the scaled image) so merged the two calls using a semicolon.

In contrast, I broke out the scaled image drawing over three lines, to emphasize the use of the calculated destination bounds. I wanted to expand the details of this operation while minimizing the background fill.

Testing

There’s some sample tests at the bottom of my version if you want to throw this into a playground and test with real data. The placeholder generator website (http://placehold.it) creates simple block images along whatever dimensions you request.

I’m interested in seeing the kinds of rewrites you came up with and what kind of Swift rules they incorporate. Disagree or agree with my edits? Please let me know. My style choices are still evolving.

Tweet this Post

23 Comments

  • I get the idea of scoping begin/end draw calls, but for Swift I think defer might be useful for managing the block, as in this gist https://gist.github.com/mbarriault/82aaee5bef05fc73919a3115a7bb7f64

    • I see what you’re getting at — using defer for cleanup is a good thing — but these calls are so clearly parallel and opening/closing that it just doesn’t read to me the way I want it to. Don’t forget that you can also push and pop contexts on the UIKit context stack as well.

      • By erica

  • This is a cool exercise! My result: .

    I tend to like longer single lines, rather than splitting statements over multiple lines, but that’s just personal preference. The main things that I’d do differently is:
    – Use != operator in the guard, which I find a lot more readable than CGSizeEqualToSize().
    – Use min(),max() for computing scaleFactor, as I find it easier to follow than your double trinary.
    – Pull out scaling a CGSize by a factor into an extension func, since it feels reusable and general-purpose.

    • Oops, stripped the url https://gist.github.com/gregomni/80db83c98293ca10917487973005de10

    • Is CGSize already equatable or would you add an extension? I do really like min and max and didn’t consider them. There’s actually quite a bit of math that would be better pulled out to CG extensions: size.fitInSize -> rect, kind of stuff

      • By erica

      • CGSize : Equatable is built in.

    • Good ever exercise. I liked your tuple-ing of the points. I too used max(). I Also prefer this as an extension of UIImage rather than a global method.

  • Ah. Looking now. I think I’d use an operator for scaled (CGSize * CGFloat) rather than a method

    • By erica

    • Yeah, I’d definitely do that (and have done that!) in places that did more computational geometry. If this image scaling was more of a standalone without a bunch of other similar stuff “near” it, then I tend to avoid operators. I think it adds an extra bit of difficulty in understanding unless you are writing code that explicitly or implicitly declares HERE THERE BE MATH.

      • I have a big math library for doing things like point + vector, rect * scale, rect + vector, etc.

        • By erica

        • ?

          • I really like your min max the more I look at your code and am kicking myself for not using them.

            • By erica

          • p.s. You can do this:

            let scaleFactor = (fitImage ? min : max)(size.width / image.size.width, size.height / image.size.height)
            • By erica

  • A nice one here too: http://twitter.com/somegeekintn/status/737689528425689088

    https://gist.github.com/somegeekintn/c2d8830e4e2440f0d4ef5a64d8e6bb0c

    • By erica

  • Well the first part of my solution differs from your in two respects: 1) I didn’t bother with the fill test, 2) I didn’t use tuples to the scale factor assignments and scaled dimension assignments to one line each.

    However, the interesting part for me was this part:

        UIGraphicsBeginImageContextWithOptions(size, false, 0.0)
    image.drawInRect(thumbnailRect)
    let scaledImage = UIGraphicsGetImageFromCurrentImageContext()!
    UIGraphicsEndImageContext()

    It seems to me that is crying out for a function along the lines of 

        func graphicsContextWithOptions(size: CGSize, opaque: Bool, scale: CGFloat, whatToDo: () -> ())
    {
	UIGraphicsBeginImageContextWithOptions(size, opaque, scale)
        whatToDo()
        UIGraphicsEndImageContext()
    }

    So you can write something like

        var scaledImage: UIImage?
    graphicsContextWithOptions(size, opaque: false, scale: 0.0) {
        image.drawInRect(thumbnailRect)
        scaledImage = UIGraphicsGetImageFromCurrentImageContext()
    }
    return scaledImage! // Force unwrap for demo purposes only

    • So WordPress screws the formatting, sorry.

      • FTFY.

        Also, I use this on my OS X calls: 

        public func ImageByDrawingBlock(targetSize: CGSize, _ block: () -> Void) throws -> NSImage {
        • By erica

      • I like that! I’d put the return value in the function and make it part of UIImage: https://gist.github.com/gregomni/91c5970230cecf3e0eaad4fe38d5d803

        • Left a comment underneath. I think there’s still some tweaking to be improved on using CGRectOffset. Will post there once I get it done

          • By erica

    • That’s great!
      I would change the signature a little bit to accommodate for a return value (and I added the bounds as a convenience parameter for the closure):

      —-
      func graphicsContextWithOptions(size: CGSize, opaque: Bool, scale: CGFloat, draw: (withinBounds: CGRect) -> T) -> T
      {
      UIGraphicsBeginImageContextWithOptions(size, opaque, scale)
      let bounds = CGRect(origin: .zero, size: size)
      let result: T = draw(withinBounds: bounds)
      UIGraphicsEndImageContext()
      return result
      }
      —-

  • My solution builds on a small framework for geometric operations which makes them much easier to read IMO.
    https://gist.github.com/trs123/e65a4b6430af327796c1c8769f14ca00

  • […] original function to suit my taste. You can find the original snippet here, my version here, and Erica's version on her blog. I also encourage you to try it yourself before looking at her or my […]

  • My version is a more plain refactor than some have posted, since I wanted to keep the comparison between original and revised without adding functionality. https://gist.github.com/rosslebeau/34967db822c3571203cedf860cb832b1

    I thought about adding fit/fill, as well as options for the crop position. I’d probably add these if this were a function I planned to use generally. You can read my comments on your version and mine here: http://rosslebeau.com/2016/swift-rewrite-challenge