Five Signs Of Code Smell In Swift

Pixelsync was the first iOS application I published on the App Store. For a first project, it was quite complex and far more challenging than I had anticipated. The project grew quickly as I added more features and maintainability quickly became an issue I could not ignore.

Looking back, the project was littered with anti-patterns, bad practices, and code smells. Adding features to a large, complex code base is challenging if it lacks direction and structure.

Many developers start out this way and learn as they go. It is fine to make mistakes as long as you learn from your mistakes and find solutions that work better. In this article, I want to focus on five signs of code smell in Swift.

1. Forced Unwrapping & Downcasting

Once you get used to optionals, you come to appreciate their value. Not only are optionals making your code safer, they also make your code more readable. An optional carries a message that says "I may not have a value. Be careful."

Developers new to Swift often see optionals as a hurdle, forcing them to jump through a bunch of unnecessary hoops. Optionals need to be unwrapped and that requires more code. There is a shortcut, though. It is possible to force unwrap optionals. Take a look at the following example.

override func prepareForSegue(segue: UIStoryboardSegue, sender: AnyObject?) {
    if segue.identifier == viewController {
        let navigationController = segue.destinationViewController as! UINavigationController
        let addViewController = navigationController.viewControllers.first as! AddViewController
        addViewController.delegate = self

This looks fine. Right? We are sure that the destination view controller of the segue is a UINavigationController instance and the first view controller of the navigation stack is, without a doubt, of type AddViewController.

It does not matter how certain you are, an optional should be treated with caution. Forced unwrapping or forced downcasting is asking for trouble. There are situations in which you can use the ! operator (outlets, for example), but do not force unwrap an optional to avoid an if statement or a few extra lines of code.

The updated example shows you how elegant optional binding can be. The if statement tells the developer what we expect, but, at the same time, it hints that we may not get what we expected.

override func prepareForSegue(segue: UIStoryboardSegue, sender: AnyObject?) {
    if segue.identifier == addItemViewController {
        if let navigationController = segue.destinationViewController as? UINavigationController,
           let addItemViewController = navigationController.viewControllers.first as? AddItemViewController {
                addItemViewController.delegate = self

If you find yourself using a ! instead of an ?, then stop for a moment and ask yourself whether there is a safer alternative.

2. Monster Classes

Andy Matuschak is a great speaker and a very good teacher. A few months ago, he gave a wonderful talk at the Realm offices about refactoring view controllers. In the talk, Andy illustrates that view controllers are often taking on far too many responsibilities.

Some of us apply the MVC (Model-View-Controller) pattern a bit too strictly. It is fine to create classes that are not models, views, or controllers.

Some developers even argue that you should create fat models and skinny controllers. While that may be a bit too much responsibility for the model layer, I agree that controllers should be lean and lightweight. Of course, it is easy to stuff business logic in controllers and it takes a bit of extra work to refactor code that can belong in a separate class. But it is often the right choice.

By isolating functionality, you put the view controllers of your project on a diet and make code easier to reuse. Why not extract a piece of functionality in an Objective-C category or a Swift extension.

If the implementation of a class exceeds 1000 lines of code, I start to hear warning bells. Can you load 1000 lines of code in your working memory? That is what it takes to work with a class. If you make changes to a class, you need to know how other parts are affected and that is only possible if you know what the various parts of the class do.

3. Ignoring Errors

It is great to see that error handling is tightly integrated into Swift. In Objective-C, it is easy to ignore errors, too easy. Have you ever seen something like this?

[managedObjectContext save:nil];

I don't know anyone who enjoys error handling. But if you want to write code that works and that can recover when things start to go wrong, then you need to accept that error handling is part of it. Ignoring errors also makes debugging more difficult. Putting your head in the sand is not the solution when things go wrong.

4. Singletons

Singletons are great. They are so useful that less experienced developers tend to overuse them. Been there, done that. I am not joking when I say that singletons are great. Even though I don't consider the singleton pattern an anti-pattern, sometimes it seems as if developers have forgotten how to pass objects by reference.

One of the negative side effects of singletons is tight coupling. Over the years, I have come across projects that were littered with singletons, singletons referencing other singletons. This can make refactoring a nightmare.

After removing the singletons from a project, it is no longer intimidating and it loses some of its complexity. By passing objects by reference, even if only one instance is alive at any one time, it becomes much clearer where the object is used and what role it plays.

Another benefit of keeping singletons to a minimum is testing. Singletons make testing more difficult.

There is nothing wrong with passing objects by reference. Whenever you are about to create a singleton, ask yourself whether passing the object by reference is an option. That should be your first choice.

5. String Literals

String literals are smelly by default. They are great in playgrounds, but how often do you (or should you) use string literals in a project? The only valid use cases I can think of are localization, constants, and logging.

I agree that creating constants or enumerations is tedious, but it pays off in the long run. You avoid typos, autocompletion kicks in, and it significantly improves maintainability.

This certainly isn't a hard rule that I apply in the projects I work on, but I try to stick to it as much as possible. If the value of an object literal is used in more than one place, it is defined as a constant or enumeration, or, even better, put in a configuration file.

My Xcode color scheme highlights string literals in bright red. When I am browsing through lines and lines of code, the string literals stand out immediately. If I see too much red, it is time to refactor.

Does Your Code Smell?

As I mentioned in the introduction, the goal of this article is to review the code you write. How can I improve this? What side effects does this implementation have? Should I force unwrap this optional? It rarely pays off to take a shortcut.

I am interested to hear what you think about code smells. Chances are that you disagree with some, or most, of my arguments. Let me know in the comments below or reach out to me on Twitter.