The first application I published on the App Store was littered with code smells, bad practices, and anti-patterns. The application worked, but it was a challenge to maintain. Adding features to a large, complex project becomes increasingly difficult if it lacks direction and structure.
Most 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 episode, I want to focus on five signs of code smell in Swift.
1. Forced Unwrapping and Forced Downcasting
Once you get used to optionals, you come to appreciate their value. Not only do optionals make your code safer, they also make your code more readable. An optional carries a message that says "I may not have a value so 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 forced unwrap optionals. Take a look at the following example.
override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
switch segue.identifier {
case Segue.addItem:
let navigationController = segue.destination as! UINavigationController
let addViewController = navigationController.viewControllers.first as! AddViewController
addViewController.delegate = self
default:
break
}
}
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 comes with a clearly defined risk. There are situations in which you can use the ! operator (outlets, for example), but I don't recommend to forced 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 guard statement tells the developer what we expect, but, at the same time, it hints that we may not get what we expected.
override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
switch segue.identifier {
case Segue.addItem:
guard
let navigationController = segue.destination as? UINavigationController,
let viewController = navigationController.viewControllers.first as? AddViewController
else {
return
}
viewController.delegate = self
default:
break
}
}
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
A few years ago, Andy Matuschak gave a talk about refactoring view controllers. In his talk, Andy illustrates that view controllers are often taking on far too many responsibilities. Many developers apply the MVC (Model-View-Controller) pattern a bit too strictly. They forget that it is fine to create classes that are not models, views, or controllers.
Some 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 a Swift extension, for example?
If the implementation of a class exceeds a thousand lines of code, I start to hear warning bells. Can you load a thousand 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. Singletonitis
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 enums is tedious, but it pays off in the long run. You avoid typos, you get autocompletion for free, 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 enum, or, even better, put in a configuration file.
The Xcode color scheme I use highlights string literals in bright red. When I am browsing through lines and lines of code, the string literals pop 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 episode is to review the code you write. How can I improve this? What side effects does this implementation have? Should I forced unwrap this optional? It rarely pays off to take a shortcut in software development.
