How to fail basic refactor using Clean Architecture
Does Bob know you are writing dumb shit in his name?
Here is a question.
How many lines of code do you think it’s reasonable to add an URL endpoint?
Clean Architecture under the collective wisdom of industry titans with over a decade of experiences decided that number is > 40. With a setup for presentation it will be > 120. That is just setup, not actual presentation.
Source: Clean Architecture and MVVM on iOS
I did it < 5 in my first project.
// in Api.swift
var endpoint = Resource("/endpoint")
// in view controller
var json: JSON { didSet {updateView()}}
// fetch call
api.endpoint.post(params).onSuccess(json in self.json = json)Value type model. Property observer to map it to view. One way to do presentation is to use computed properties:
var title: String { json["Result"]["Title"].stringValue }For reuse you can refactor it out as a protocol extension
protocol Presenter {
var json: JSON {get set}
}
extension Presenter {
var title: String { json["Result"]["Title"].stringValue }
}
// self-contained with default impl, unlike Java protocol All these come together to SPEED UP the workflow, using Swift language features with RESPECT to SDK. And I only use pre-defined types. I can use the same types for every endpoint.
Clean architecture has to beat this, or at least has some advantages over it.
Does it? Bob didn’t know Swift. MVVM didn’t consider value type. The article has no mention of value type, property observer nor any comparison. It used protocol like Java. In fact it is written like Java.
What I’m going to do here is to debunk his “conclusion”:
More software engineering best practices:
Do not write code without tests (try TDD)
Do continuous refactoring
Do not over-engineer and be pragmatic
Avoid using third-party framework dependencies in your project as much as you can
That is, he over-engineered, didn’t refactor for shit, and in fact needs to write a lot more tests to cover his boilerplate.
Some disclaimer first: I didn’t read Bob’s book. But from the work of his pupils, I don’t think it works on any practical level.
Do not over-engineer and be pragmatic
Let’s start with this snippet:
protocol SearchMoviesUseCase {
func execute(requestValue: SearchMoviesUseCaseRequestValue,
completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable?
}
final class DefaultSearchMoviesUseCase: SearchMoviesUseCase {
private let moviesRepository: MoviesRepository
private let moviesQueriesRepository: MoviesQueriesRepository
init(moviesRepository: MoviesRepository, moviesQueriesRepository: MoviesQueriesRepository) {
self.moviesRepository = moviesRepository
self.moviesQueriesRepository = moviesQueriesRepository
}
func execute(requestValue: SearchMoviesUseCaseRequestValue,
completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable? {
return moviesRepository.fetchMoviesList(query: requestValue.query, page: requestValue.page) { result in
if case .success = result {
self.moviesQueriesRepository.saveRecentQuery(query: requestValue.query) { _ in }
}
completion(result)
}
}
}This is a fetch request. Where’s network service?
It’s hidden under Repository, which is supposed to be a reference type model.
Why reference type model when you can have value type? The implication is that you have to refactor out mutable side effects in Swift. It’s more strict, that’s why it is safer. A good Swift dev makes mutable side effects explict, via protocol requirement or other means, not hidden via nested injection.
From this point on, you need a whole new set of thinking. We are not in Kansas(Java) anymore.
You inject MoviesRepository because you want to swap it out at runtime for…
protocol MoviesRepository {
func fetchMoviesList(query: MovieQuery, page: Int, completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable?
}Which means, in order to have some parameter changes for a fetch, e.g.; endpoint or callback handler, you need to write a new class from scratch.
final class DefaultMoviesRepository {
private let dataTransferService: DataTransfer
init(dataTransferService: DataTransfer) {
self.dataTransferService = dataTransferService
}
}extension DefaultMoviesRepository: MoviesRepository {
public func fetchMoviesList(query: MovieQuery, page: Int, completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable? {
let endpoint = APIEndpoints.getMovies(with: MoviesRequestDTO(query: query.query,
page: page))
return dataTransferService.request(with: endpoint) { (response: Result<MoviesResponseDTO, Error>) in
switch response {
case .success(let moviesResponseDTO):
completion(.success(moviesResponseDTO.toDomain()))
case .failure(let error):
completion(.failure(error))
}
}
}
}Let’s pause and ponder for a moment.
If you want to change endpoint, you can pass it as a function parameter without writing a whole new class. E.g.; api.fetch(“/newEndpoint”)
But he can’t. Because he hard-coded endpoint in APIEndpoints.getMovies(DTOShit(...))
The one parameter you most likely would need, he doesn’t have it.
Instead he went through all the trouble of injecting some DTO shit, which basically turns a response to a Codable . Then again, you are expecting to swap it out at runtime for… ? Another Codable type from the same response? So once again, you have to create a new class from scratch for another type because it’s hard-coded?
And note that, he hard-coded all these specific to one endpoint, he has to start from scratch AGAIN for another endpoint. It’s a deep-nested process with insane overheads! E.g.;
ViewModel(UseCase(Repository(DataTransferService(APIEndpoints(...), ...))))
Each of which requires a protocol, a default implementation, and any variation requires a news class from scratch.
You see problem(s) here?
Clean Architecture under the collective wisdom of industry titans with over a decade of experiences, doesn’t know how to use variable or pass parameter. And it doesn’t give a shit about anything other than injection.
When in practice, injeciton is NOT a priority. As the case here, the flexibility to pass parameters is far more useful. What’s the point of injection when all it does is pass parameters? And in a shit-y way! What if you don’t use DTO injection shit and use plain old function?
static func getMovieData(_ json: JSON) -> MovieData
static func getMovieArray(_ json: JSON) -> MovieArrayObviously you can then improve this by using generics, I leave it as an exercise.
This highlights another key point: function over object. This is a consequence of having value type, and we are breaking away from OOP. Not everything needs to be an object, which is abused in MVVM. These functions can be easily extended in protocol, hence POP.
The icing on the cake is this
final class DefaultMoviesRepository {...}It’s final . Nobody can reuse your “default”. So when they start from scratch, they literally start from scratch, because protocol has no default implementation, and defaut implementation is fianl . They have to copy paste your shit, otherwise they are just re-doing most of your work.
It’s time to remind ourselves what Clean Architecture is all about.
Do not over-engineer and be pragmatic
Did he succeed? No.
There’s nothing pragmatic about this. It’s easy when you can ignore all the nested injection, type declaration overheads, and object interactions.
None of these injections have meaningful purpose. They just do the same thing with slight variation, which can be done in far simpler ways.
None of the dependency shit has practical use here. In use case like this, you don’t need a design for N network services, M DTOs, K repositories, that can be swapped during runtime.
You need ONE that WORKS EFFICIENTLY. Get one right before generalizing it. He can’t even get ONE right!
Do you notice he didn’t bother discussing why his use case needs it? As it turns out it doesn’t! This is the danger of following abstract principles from a book published a decade ago.
Now if you want to build a system that supports swapping N objects during runtime, they better be refactored to perfection. Otherwise you are going to copy paste N times.
Is it? Refacored to perfection?
Do continuous refactoring
Take this for example:
return dataTransferService.request(with: endpoint) { (response: Result<MoviesResponseDTO, Error>) in
switch response {
case .success(let moviesResponseDTO):
completion(.success(moviesResponseDTO.toDomain()))
case .failure(let error):
completion(.failure(error))
}
}How many times do you need to repeat the same swtich case ? You design to have N DTOs, remember?
The .failure case will be the same in almost all cases. This brings us to yet another problem: where’s basic refactor?
Isn’t Clean Architecture supposed to be held up to highest standards?
Nah, Clean Architecture and MVVM is somehow above the law. They are not bound by overheads or refactor. They have Bob and his magnum opus.
Let’s take a look at protocol design.
protocol MoviesRepository {
func fetchMoviesList(query: MovieQuery, page: Int, completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable?
}
protocol MoviesQueriesRepository {
func fetchRecentsQueries(maxCount: Int, completion: @escaping (Result<[MovieQuery], Error>) -> Void)
func saveRecentQuery(query: MovieQuery, completion: @escaping (Result<MovieQuery, Error>) -> Void)
}There’s this insistency to hide networking for no reason in MVVM. Because they can’t have control. It’s MVC and it’s bad for selling tutorial.
All of these can be refactored into networking. E.g.;
func fetch(_ endpoint: String, _ params: JSON, _ callback: (JSON) -> ())There’s no point to conform to these protocols. What are you going to do?
func fetchMoviesList(query: MovieQuery, page: Int, completion: @escaping (Result<MoviesPage, Error>) -> Void) -> Cancellable?Fetch the same move list again?
What you need is library functions with enough flexibility for various networking task. You can refactor it out to protocol, provided you have extension, so it becomes library functions, not requirement. Who is going to implement this shit again? Any sane person would copy paste. E.g.;
protocol MovieFetch: class {
var api: NetworkService {get set}
var json: JSON {get set}
}
extenstion MovieFetch {
func fetchMoviesList(query: MovieQuery, page: Int) {api.fetch(...)} // update JSON
}I’d avoid putting @escaping (Result<MoviesPage, Error>) -> Void) in signature, because it’s just a wrapper to pass it to network service, which is redundant and you need to repeat it for another fetch like fetchRecentsQueries . This usually works because again, you don’t need that much runtime capability to pass a closure. What you are going to do with a fetch can be determined in compile time. You need a function, and have default implementation, which we do. Know the problem you are working on so you can optimize it.
Since your requirement consists of a network service, and json as response data, you can see that it natually builds around it. This is why Swift is POP.
api and json are front and center. class means mutable. Extension because you have to refactor out mutable functions out from model so model, i.e.; json is value type. Make it self-contained so they are reusble as mix-in. Start with concrete types not abstract principles, and only add more if needed.
You know, basic refactor.
If you are wondering why Clean Architecture is at the bottom of progression, it can’t even do such a basic flow. Is it possible to learn this power? Not from MVVM, and least of all Clean Architecture.
Speaking of MVVM, we haven’t even covered view model yet.
This is binding.
override func viewDidLoad() {
super.viewDidLoad()
bind(to: viewModel)
viewModel.viewDidLoad()
}How did he do binding?
public final class Observable<Value> {
private var closure: ((Value) -> ())?
public var value: Value {
didSet { closure?(value) } // <------------- HERE!
}
public init(_ value: Value) {
self.value = value
}
public func observe(_ closure: @escaping (Value) -> Void) {
self.closure = closure
closure(value)
}
}Property observer.
What’s the difference between this and my approach at the start?
He had to delegate it outside to a view model object, whereas I didn’t.
Do we accomplish the same thing? Yes. He had to take a detour because he has to have a view model object. Obvisouly he is going to ignore any overhead costs, and talks about unit test…
Basic refactor.
If there’s an easier to do the same thing, refactor. E.g.;
viewModel.items.observe(on: self) { [weak self] in self?.tableViewController?.items = $0 }This becomes
var items = [Item]() { didSet { tableViewController.items = items }}Which you can see require another refactor! Because a wrapper that only pass data is usually a code smell. I leave it as an exercise.
What’s the wisdom again? “Continuous refactor”? Which is odd, coming from a guy who didn’t bother at all.
If you argue that I’d be having massive view controller, then I’d argue I can refactor out things if needed with extension. What ground breaking techniques does MVVM offer in a SDK that does NOT support binding anyway? You create objects and put things there too! It’s not like the rest of us don’t know how to CREATE OBJECTS. Let’s see how MVVM avoids massive view cotroller.
// Step 1: Define action closure to communicate to another ViewModel, e.g. here we notify MovieList when query is selected
typealias MoviesQueryListViewModelDidSelectAction = (MovieQuery) -> Void
// Step 2: Call action closure when needed
class MoviesQueryListViewModel {
init(didSelect: MoviesQueryListViewModelDidSelectAction? = nil) {
self.didSelect = didSelect
}
func didSelect(item: MoviesQueryListItemViewModel) {
didSelect?(MovieQuery(query: item.query))
}
}He passes closures to view model as event handler.
Pleasse tell me how many times he repeats didSelect , and ViewModel .
Not to mention you have to pass closure, which is harder to track than functions, because it can be assigned from anywhere.
Just from this code snippet, you have no idea what MoviesQueryListViewModelDidSelectAction really is. Hell I give up reading the word half way. If I search where it is assigned, don’t tell me I’ll find it in view controller.
func makeMoviesQueriesSuggestionsListViewController(didSelect: @escaping MoviesQueryListViewModelDidSelectAction) -> UIViewController {
if #available(iOS 13.0, *) { // SwiftUI
let view = MoviesQueryListView(viewModelWrapper: makeMoviesQueryListViewModelWrapper(didSelect: didSelect))
return UIHostingController(rootView: view)
} else { // UIKit
return MoviesQueriesTableViewController.create(with: makeMoviesQueryListViewModel(didSelect: didSelect))
}
}Thank god it’s not. It is in the Dependency Injection Container (DIC) that makes view controller.
Wait, what?
A singleton that can be accessed by anyone providing a function to make a view controller that takes a view model as a parameter which has a wrapper that takes a closure that can be anything.
That is easy to read, easy to track, easy to test, and easy to maintain.
MoviesQueryListViewModelDidSelectAction ? It’s passed in from singleton. Duh?
We still don’t know what it does btw. It takes hard work and real talent to try and build a more useless architecture.
Genious work. Really. Be proud of yourself, Bob.
It’s funny because encapsulation is one of the pillars of OOP, and these guys don’t seem to give a shit. Imagine view controller not knowing what didSelect does. It has to be told by an external entity whose identity remains a mystery.
Unlike Clean Architecture, which operates solely on theory and abstract principles. i.e.;
We work on codes. Do I explain SOLID principle and dependency management first and build solely on them regardless of the problem?
No. We add what we need. And refactor along the way. And most importantly, we don’t write 0 refactor shit and publish it as “clean”, and lecture people about it.
Alright, I’ve got to do hair cut in an hour, so I’m gonna skip this overhead mess:
private var viewModel: MoviesListItemViewModel! { didSet { unbind(from: oldValue) } }
func fill(with viewModel: MoviesListItemViewModel) {
self.viewModel = viewModel
bind(to: viewModel)
}
private func bind(to viewModel: MoviesListItemViewModel) {
viewModel.posterImage.observe(on: self) { [weak self] in self?.imageView.image = $0.flatMap(UIImage.init) }
}
private func unbind(from item: MoviesListItemViewModel?) {
item?.posterImage.remove(observer: self)
}Binding is not free is UIKit. Force unwrap is code smell. My standard is that it is only allowed for @IBObservable, which you don’t have choice. But it’s Clean Architecture so it can have lowest standards.
I'm going to skip all the setup codes here and there, which are hard to test and prone to human errors. Oh yes, test.
Do not write code without tests (try TDD)
MVVM devs are automatic TDD titans. But let’s try fact-checking them.
What is the proof or comparison that this is easier to test?
What are the objective facts here?
He used way more protocols and classes, he had a lot of imperative assignments, he used a lot of closures. He used singleton. Every instance came with non-trival initializer that are NESTED. For view model you have to setup binding which involves view, hence hard to unit test.
For a same fetch function, he had to test N times with different classes and initializers. As a comparison, I use the same function for every fetch, so I test the function in library test, test parameters and responses in development. Even without writing any unit test, I know it would work reasonably well without edge cases. I will try edge cases if I have the time. The priority is lower.
Unlike TDD titans, we average devs don’t have the kind of skill to write bullet-proof tests easily. A lot of time could go into waste and still can’t find the bug.
Can you really say Clean Architecture, from what we’ve seen so far, provide any advantages in testing? (we know how to create object too)
His conclusion is:
MVVM and Clean Architecture can be used separately of course, but MVVM provides separation of concerns only inside the Presentation Layer, whereas Clean Architecture splits your code into modular layers that can be easily tested, reused, and understood.
Let me remind Clean Architecture again that it needs to beat this:
// in Api.swift
var endpoint = Resource("/endpoint")
// in view controller
var json: JSON { didSet {updateView()}}
// fetch call
api.endpoint.post(params).onSuccess(json in self.json = json)Easily tested, reused, and understood.
Let me say that again, with the code review we just had in mind.
Easily tested, reused, and understood.
Than this? I’ll leave the judgement to you.
Let’s wrap up.
Learn some basic refactor, please
That article is the top google result for Swift Clean Architecture with 3.6K claps; His repo has almost 3K stars.
My opinion? Dude, learn some basic refacor, or basic Swift features.
I particularly like this gem from the comment section:
In the session “What data crosses the boundaries”, the Entity should not crosses the boundary and only be dependent by Use Case. Therefore, the Interface Adapter should not dependent on the Entity: the ViewModel should not know the classes (Movie / MovieQuery) in the Entity.
But if I transform the data from the Entity to DTO, it would break the DRY principle easily.
However, I like you solution that you group the Entity and Use Case layers into one bigger Domain layer.
Dude, you get all that from a fetch?
Here’s an idea, maybe instead worrying about “boundary”, “use case”, “interface adapter”, DTO, DRY principle, domain layer… etc, you can uh… what’s the word…
REFACTOR YOUR SHIT!
BUILD A HALF-DECENT NETWORK SERVICE FIRST!
I’m going to quote from House. M.D.
Dr. Gregory House : You wake up in the morning, your paint’s peeling, your curtains are gone and the water is boiling. Which problem do you deal with first?
Dr. Eric Foreman : House!
Dr. Gregory House : None of them! The building’s on fire!
You wake up in the morning, your “entity” crosses the “boundary”, your “adapter” depends on “entity”, your view model knows “the class”, which problem do you deal with first?
None of them!
Your networking is dog shit!
You don’t have basic refactor!
Your code is boilerplate mess!
I hope by now you can appreciate why Clean Architecture ranks the shit-est iOS skill in the progression chart.
There’s a lot more interesting things that I can say about this chart, perhaps after I’m done playing Diablo 4.
