ParkenDD is an app that shows you parking spots available in different cities in Germany. Take a simple function from their project.
- There’s an extraneous return in the failure case.
- Using nil coalescing operators in general is a code smell to me, so most of the success case I’d change.
- The flatMap should be much more readable by removing the return type, clearing the guard and explaining how come a city may return nil.
If I were the one reviewing this code, it wouldn’t pass code review. All of these issues should be fixed, but, in the grand scheme of things, they’re only syntax and styling changes. What I’d really like to fix is not visible from a superficial examination.
First, what is the function actually doing?
- Creating a Data task and sending it to the API (via
fetchJSON), then validating whether it succeeded or failed.
- Validating the JSON by making sure it contains a “cities” array.
- Transforms the validated JSON into City objects, then sorts them.
If I had to simplify it, I’d say it’s serving two different purposes:
- Coordinating the networking call and parsing of said call.
- Formatting of the result
This function is both deciding where to get the cities and applying business logic to them. Business logic is the algorithms that make your app distinct. It’s the sauce of the app. Right now this function is mudding the waters between the pure model which comes from the API and what to do with that model.
I see this all the time. I honestly cannot emphasize how often I see a function that mixes concerns. It’s difficult to see, but let’s see who is calling
In traditional MVC, the VC’s Single Responsibility is to coordinate the view with the API. It should receive the cities, get them ready for the table view and pass them along when they’re ready. From the looks of it, it’s doing exactly that.
The API should be platform-agnostic. Its Single Responsibility is to be the source of truth. It should not care if you’re presenting the cities on a map, on a table view, or on a single label by concatenating all of them. From the looks of it, it’s doing exactly that.
Can you spot the issue?
A designer/product person/stakeholder asks you to please change the sorting of cities. Instead of sorting them alphabetically they’d really like to sort them by distance.
Where do you make the change? In
fetchCities who is responsible for fetching them from the API or in
CitySelectionTVC who is responsible for displaying them?
This is the whole premise behind the Single Responsibility Principle: make sure that any changes requested go to the single, adequate place in your codebase.
The sorting of cities in ParkenDD is in a single place, but it’s not in the right place and that is the #1 reason for bugs! There is this pervasive meme where you touch one part of the code and a bunch of bugs appear elsewhere. The unending game of whack-a-mole. The reason this happens is because of tight coupling that could be prevented with the Single Responsibility Principle.
I’ve chosen a very simple example because it’s also a very subtle one. The solution, now that you know that
fetchCities is mixing concerns is to move the sorting to where it belongs.
It’s not a big deal
Every time I explain the SRP to a junior I get told that it doesn’t seem like a big deal, and most of the time it isn’t a huge deal. Code is code. If it works and ships, that’s great.
But one day you will be responsible for the whole codebase or for a big chunk of it and other people will have to work with you. You’ll quickly realize that reducing the amount and severity of bugs is easy if you respect the Single Responsibility Principle.
If you liked this article, please consider subscribing! I write a weekly newsletter with a 15-minute coding exercise, and additional sections like interviews from members of our community.