I recently interviewed at a company, and they gave me some code to review. I did my best to ignore the proctor’s deliberately slow speech tempo. He paused for non-existent questions. The code appeared to me to be self-explanatory, just rife with mistakes, code smells, and misuse of design patterns.
I didn’t know where to begin with my issues with the code. I estimate it would be a weeks-long discussion or an ongoing debate over months because this developer seemed to have deeply ingrained notions about it. He made a lot of assumptions about my perspectives.
In his defense, these practices are relatively common in the professional community and cause many headaches. This is why I think an article is maybe the most appropriate response.
Let’s look at this Ray Wanderlich tutorial. This isn’t a dig on the developer that wrote this article at an unknown date, potentially years ago, and with unknown requirements to me. My article is just a dig on the practices in the RW article, nothing more.
My biggest gripes are: the unnecessary complexity puts a burden of overhead study onto devs, new and seasoned, and because of this complexity, it makes it a real headache to debug when your API request goes wrong.
Most importantly, following a design pattern doesn’t give your code purpose. You may just be using a design pattern to optimize something that had no reason to exist in the first place.
It’s about to get juicy. Suppose you disagree with anything I say or feel I’m underselling these practices’ benefits. Please feel free to comment and participate in the conversation.
It starts with this:
protocol RequestProtocol {
// 1
var path: String { get }
// 2
var headers: [String: String] { get }
var params: [String: Any] { get }
// 3
var urlParams: [String: String?] { get }
// 4
var addAuthorizationToken: Bool { get }
// 5
var requestType: RequestType { get }
}New type count, their way (1), my way (0)
Because you can combine protocols with type aliases, I am not a fan of protocols having more than one required property or method. They create the problem of requiring implementations that aren’t needed, that return empty arrays, zeros, and empty dictionaries, with the hope that other developers don’t accidentally use these silly implementations, and therefore with the prospect of protocol extensions, providing functionality that shouldn’t be available.
Later in the tutorial, it creates an extension for this protocol.
extension RequestProtocol { // 1
var host: String {
APIConstants.host
}
// 2
var addAuthorizationToken: Bool {
true
} // 3
var params: [String: Any] {
[:]
}
var urlParams: [String: String?] {
[:]
}
var headers: [String: String] {
[:]
}
}
URLRequest already has defaults for headers, URL params, etc.
This protocol appears to have no reason to exist. URLRequest already provides this functionality.
That brings me to one of my software principles:
1. Don't create types that have no reason to exist.
The tutorial goes on to imitate the wheel further.
enum RequestType: String {
case GET
case POST
}New type count, their way (2), my way (1)
This enum has a reason to exist because the HTTP method property on URLRequest is “Stringly” typed, meaning you have to assign a string version of “POST” to it, and if you make a typo and instead assign “Pest,” the compiler won’t help you. I can respect this enum.
However, looking forward, the URLRequest property is written to anyway.
urlRequest.httpMethod = requestType.rawValueCreating this enum doesn’t prevent developers from assigning a stringly typed value to the HTTP method. A raw string value is used anyway, except through a technique in which a developer can sidestep these methods. It only adds safety when added to an upcoming method signature. Nothing is enforcing that other devs call that function. Alternatively, we can extend String and get a cleaner syntax.
extension String {
static var post: String { "POST" }
static var get: String { "GET" }
}Now you might say, “but Scott, using an enum ensures anyone that calls this method doesn’t pass a string that has a typo. Fair point. However, not adequate to outweigh the trade-offs, in my opinion. However, nothing in the code enforces that developers use this method; if they do, it may be hard to debug because of the complexity. The technique below shouldn’t exist, along with the protocol it lives in. The tutorial suggests that you add this function to an extension of the protocol.
// 1
func createURLRequest(authToken: String) throws -> URLRequest {
// 2
var components = URLComponents()
components.scheme = "https"
components.host = host
components.path = path
// 3
if !urlParams.isEmpty {
components.queryItems = urlParams.map {
URLQueryItem(name: $0, value: $1)
}
}
guard let url = components.url
else { throw NetworkError.invalidURL }
// 4
var urlRequest = URLRequest(url: url)
urlRequest.httpMethod = requestType.rawValue
// 5
if !headers.isEmpty {
urlRequest.allHTTPHeaderFields = headers
}
// 6
if addAuthorizationToken {
urlRequest.f
// 8
if !params.isEmpty {
urlRequest.httpBody = try JSONSerialization.data(
withJSONObject: params)
}
return urlRequest
}Here they assign values from the new types that don’t need to exist to the URLRequest instance. URLRequest already exists. Instead of creating RequestProtocol and defining a new Struct that conforms to it, you can assign its values to URLRequest what’s wrong with setting values to a URLRequest? You can use static vars to get the reusability benefits and syntax.
extension URLRequest { static var cars: URLRequest {
// init a url request and return it.
// safely assign the httpMethod with .get or .post
}
}
You can use it like this.
let carRequest: URLRequest = .carsYou can use a static function, too, if you want to pass arguments at runtime to create the URLRequest. This solution is atomic, testable, and intuitive if you have trouble finding “where to put this code” if not in a new type.
“What about the repetitious code this will result” you might say. Every request implementation will have to implement numbers 6 and 7. Encapsulation and default arguments to the rescue.
extension URLRequest { func setAuth(token: String = <Your authToken>) {
setValue(token, forHTTPHeaderField: "Authorization")
} func setDefaultHeaders() {
setValue("application/json", forHTTPHeaderField: "Content-Type")
}
}
Plain old function to the rescue. You can do the same with queryItems and html body.
extension Dictionary where Key == String, Value == String { var urlQueryItems: [URLQueryItem] {
map { URLQueryItem(name: $0, value: $1) }
}
}extension Dictionary where Key == String, Value == String? { var urlQueryItems: [URLQueryItem] {
map { URLQueryItem(name: $0, value: $1) }
}
}extension Dictionary where Key == String, Value == Any {
func jsonData() throws -> Data {
try JSONSerialization.data(withJSONObject: self)
}
}
Suppose you want to create an easy-to-use one-stop shop to make a URLRequest predictably instead of using createURLRequest on RequestProtocol. Why not pass those protocol properties in as arguments to a URLRequest constructor?
extension URLRequest { static func standard(
authToken: String,
requestType: RequestType, // I can respect this
path: String,
host: String = APIConstants.host,
addAuthorizationToken: Bool = true,
params: [String: Any] = [:],
urlParams: [String: String?] = [:],
headers: [String: String] = [:]
) throws -> URLRequest {
var components = URLComponents()
components.scheme = "https"
components.host = host
components.path = path
components.queryItems = urlParams.queryItems
guard let url = components.url else {
throw NetworkError.invalidURL
}
var urlRequest = URLRequest(url: url)
urlRequest.httpMethod = requestType.rawValue
if !headers.isEmpty {
urlRequest.allHTTPHeaderFields = headers
}
if addAuthorizationToken {
urlRequest.setAuth(token: authToken)
} urlRequest.setDefaultHeaders() if !params.isEmpty {
urlRequest.httpBody = try params.jsonData()
} return urlRequest
}
}
The article goes on to explain the concept of async/await. This segment is excellent and informative. After that, the paper introduces another redundant protocol.
protocol APIManagerProtocol {
func perform(_ request: RequestProtocol, authToken: String) async throws -> Data
}New type count, their way (3), my way (1)
Here is another redundant class.
class APIManager: APIManagerProtocol {
private let urlSession: URLSession
init(urlSession: URLSession = URLSession.shared) {
self.urlSession = urlSession
}
}New type count, their way (4), my way (1)
This one should be a bit more obvious than the first. Classes and Structs group together otherwise disjointed properties and or methods. This class has only one property. Why not just work with URLSession, then?
The tutorial tells you to put the following method inside the new class.
func perform(_ request: RequestProtocol,
authToken: String = "") async throws -> Data {
// 1
let (data, response) = try await urlSession.data(for: request.createURLRequest(authToken: authToken))
// 2
guard let httpResponse = response as? HTTPURLResponse,
httpResponse.statusCode == 200
else {
// 3
throw NetworkError.invalidServerResponse
}
return data
}Since the class only had one property, we may as well work with that property’s type instead.
extension URLSession { func perform(request: URLRequest) async throws -> Data {
let (data, response) = try await data(for: request)
guard let httpResponse = response as? HTTPURLResponse,
httpResponse.statusCode == 200 else {
throw NetworkError.invalidServerResponse
}
return data
}
}
No need for the new classes and no need for the new protocol. Lets give other devs less work in order to understand our code. Let’s make it easier on other devs we work with.
Since URLSession usually uses the shared instance, and perform only takes one argument, we can create an extension for the single argument URLRequest.
extension URLRequest { func perform(
urlSession: URLSession = .shared
) async throws -> Data {
let (data, response) = try await data(for: self)
guard let httpResponse = response as? HTTPURLResponse,
httpResponse.statusCode == 200 else {
throw NetworkError.invalidServerResponse
}
return data
}
}
Before he continues, he appropriately calls the following section:
Layering on top of the brain
He creates new redundant classes that compose the other redundant classes. It’s redundancy on top of redundancy for free. Before that, he gives us RequestManagerProtocol.
protocol RequestManagerProtocol {
func perform<T: Decodable>(_ request: RequestProtocol) async throws -> T
}New type count, their way (5), my way (1)
Then:
class RequestManager: RequestManagerProtocol {
let apiManager: APIManagerProtocol
let parser: DataParserProtocol
// 1 init(
apiManager: APIManagerProtocol = APIManager(),
parser: DataParserProtocol = DataParser()
// 2
) {
self.apiManager = apiManager
self.parser = parser
} func perform<T: Decodable>(_ request: RequestProtocol) async throws -> T {
// 3
let data = try await apiManager.perform(request, authToken: "")
}
}
I’m not too fond of it when devs do this. We have a function named perform, calling another function perform. It’s a ripe opportunity to get confused. If someone calls one, which perform did they call? You’ll have to waste time checking. If you are looking at the perform called by perform, then you might not immediately be aware that there is another perform that might be a better candidate for the issue you are trying to solve and the feature you are trying to create. It violates the principle of “do not repeat yourself.” Some think it simplifies a long function to move its body into another method. However, when you do that, the code is no longer in a precise place. The total line count is greater, and you give your reader two function signatures to deal with instead of one.
Let’s look at the perform method that calls the other perform method. It’s almost clever. It is an attempt to deal with the fact that there isn’t a type-safe way to translate a URL string to the Codable type that corresponds to it. It doesn’t work, though. Let’s say request a returns type A. Request b returns type B. It’s possible to pass request a but specify a return type of B and vice versa.
They omitted the conversion from data to the decodable type. If you want a method to convert Data to your Codable type, you can do something like this.
extension Data {
func decodable<T: Decodable>() throws -> T {
try JSONDecoder().decode(T.self, from: self)
}
}It doesn’t pretend to provide safety when safety can’t be provided. It does one thing. No new types are needed.
New type count, their way (6), my way (1)
This class doesn’t need to exist, either. We have already implemented performance.
He then goes on to use this system, and instead of just creating instances of URLRequest, he makes a whole new type for every request.
// 1
enum AuthTokenRequest: RequestProtocol {
case auth
// 2
var path: String {
"/v2/oauth2/token"
}
// 3
var params: [String: Any] {
[
"grant_type": APIConstants.grantType,
"client_id": APIConstants.clientId,
"client_secret": APIConstants.clientSecret
]
}
// 4
var addAuthorizationToken: Bool {
false
}
// 5
var requestType: RequestType {
.POST
}
}There is another illusion of safety. With an enum like this, you have to switch on the types to have any compiler support, which would increase the line count of this type by quite a lot.
New type count, their way (7), my way (1)
Using our URLRequest constructor from before, we only need to do this:
extension URLRequest { static var authToken: URLRequest {
.standard(
authToken: "",
requestType: .POST,
path: "/v2/oauth2/token",
addAuthorizationToken: false,
params: [
"grant_type": APIConstants.grantType,
"client_id": APIConstants.clientId,
"client_secret": APIConstants.clientSecret
]
)
}
He goes on to introduce:
protocol APIManagerProtocol {
func perform(_ request: RequestProtocol, authToken: String) async throws -> Data
func requestToken() async throws -> Data
}New type count, their way (8), my way (1)
It never ends with “Classes on protocols on classes on protocols” for no reason other than to augment objects that already do all the work.
It suggests we add the following to APIManger.
func requestToken() async throws -> Data {
try await perform(AuthTokenRequest.auth)
}
let authData = apiManager.requestToken()Remember, instead of APIManager, we can use URLRequest because URLRequest is the only property on APIManager. So instead of adding a new function toAPIManager we can call a method we already have:
let authData = try await URLSession.shared.perform(request: .authToken)or
let authData = try await URLRequest.authToken.perform()Does the latter look more familiar? That is because it is more familiar. It’s nice to work with Types you are familiar with beyond your time at your current company. It speeds up developer integration.
It’s a clean, intuitive syntax that doesn’t require much research on the part of another developer.
Some misguided developers might say: “But look, let authData = apiManager.requestToken() is shorter than both let authData = try await URLSession.shared.perform(request: .authToken) and let authData = try await URLRequest.authToken.perform() .
Well, wait a second. Is it, though? Think what kind of effort is required to understand let authData = apiManager.requestToken(). Think of all 8 types you’d have to study vs the 1 for the later. Think of all the function bodies you’d have to look at, jumping to implementations down a rabbit hole of irritation vs jumping to a URLRequest extension and then jumping back twice.
The article adds another function:
func requestAccessToken() async throws -> String {
// 1
let data = try await apiManager.requestToken()
// 2
let token: APIToken = try parser.parse(data: data)
// 3
return token.bearerAccessToken
}Summary of my method of evaluation:
Stack depths: When functions, methods, computed properties, and closures are called or read, the system stores them in a data structure called a Stack. Stacks have a LIFO system of adding and removing elements. When the scope of the final function ends, it is the first to be removed from the stack. When a function calls a function calls a function, it creates an experience for a developer of tumbling down a rabbit hole of complexity. It can be tiresome as you must keep your place and remember where you are in the call stack. It creates anxiety as you wonder to yourself, how far down does this rabbit hole go? I only want to fix this tiny bug. Sometimes a certain stack depth is hard to avoid. However, when all other things are equal, a program with shallower stack traces is better than one with deeper or longer stack traces. Note it’s better to have multiple short stacks that return to the original implementation than to have fewer but deeper stacks.
Lines: Some say fewer lines are always better, and others say fewer lines say nothing. I disagree with both. Sometimes a program needs a certain amount of lines, or it’s difficult at least to avoid. Alternatively, to the people that say the number of lines means nothing, let me take this example to the extreme to prove a point. If you had to answer the question, “How did Boromir die in the lord of the rings?” Would you rather that you have to read all of book number 1? Or would you instead read the sentence directly: “Boromir died protecting Merry and Pippin.” Which do you think would take up more of your time? While sometimes several lines are needed for clarity, if all else is equal, including equal clarity, fewer lines of code are better to deal with than more lines.
Choices: Choices can be exhausting. When writing code, you create decision pathways or choices for other developers. There is a word for fatigue from decisions. It is called decision fatigue. Decision fatigue has serious ramifications. It can worsen the decision-making quality for the remainder of the day. I prefer reading ten lines of code rather than choosing which function or property stack +1 to explore.
More objects to study and understand mean more complexity if all else is the same.
Counting method
Stack depths: Stacks form nodes in a tree, then I would have to look at a stack trace for every leaf. So if a function A calls B and C and C calls D, I would have two leaves, and the stack depth score would be [2, 4]. The median would be 3. Lower medians are better than higher medians.
Lines: The number of lines of code. Let me repeat, if all else is equal, line count matters.
Choices: A list of branch choices. If you have two options, it will add 2. If you have two choices, then three, then [2, 3].
Let’s evaluate the alternative I’ve provided.
Researched Stack depths [], lines 0, choices [] types 0
let authData = try await URLRequest.authToken.perform()
Researched Stack depths [], lines 1, choices [2] types 0
let’s check out auth token. Most devs are somewhat familiar with URLRequest and other common features, so I won’t count those.Researched Stack depths [1], lines 0, choices [2] types 0
extension URLRequest {static var authToken: URLRequest {
.standard(
authToken: "",
requestType: .POST,
path: "/v2/oauth2/token",
addAuthorizationToken: false,
params: [
"grant_type": APIConstants.grantType,
"client_id": APIConstants.clientId,
"client_secret": APIConstants.clientSecret
]
)
}
Let’s check out .standard
Researched Stack depths [2], lines 12, choices [2] types 0
for posterity, I won’t paste it here. I’ll update the counts.
Researched Stack depths [2], lines 40, choices [2] types 1 (the enum)
That is the end of the line of new types to explore. Standard deals with standardized types and features. We can jump back up the stack and now check out the performance. He uses hard-coded values to create the authToken URLRequest.
let authData = try await URLRequest.authToken.perform()let’s check out perform(now.
Researched Stack depths [2], lines 40, choices [2] types 1
extension URLRequest {func perform(
urlSession: URLSession = .shared
) async throws -> Data {
let (data, response) = try await data(for: self)
guard let httpResponse = response as? HTTPURLResponse,
httpResponse.statusCode == 200 else {
throw NetworkError.invalidServerResponse
}
return data
}
}
RESULT: Researched Stack depths [2, 1], lines 50, choices [2] types 1
Done.
Let’s evaluate the typical method provided by Ray Wanderlich.
Objects: Count new objects.
Researched Stack depths [], lines 0, choices [] objects
Let’s see. How can we call perform? To figure that out, I have first to study 8 objects + some objects that I think we can only see if we pay for a project to download:
parser: DataParserProtocol = DataParser()So
Researched Stack depths [] ✅, lines 0 ✅, choices [] ✅ objects 10 ❌
Already the objects of this method exceed the other.
But after that,
RequestManager(apiManager: .init(), parser: .init()).perform(AuthTokenRequest.auth)To start off we have RequestManager (1 type), APIManager (2 types), DataParser (3 types), and AuthTokenRequest (4 types).
Researched Stack depths [0], lines 1, choices [5], objects 4
Already we have an extensive choice of 5 different stacks to explore.
Let’s check out AuthTokenRequest.auth. It’s an enum with one case. That feels odd. Not too fun to switch over, but when you create a protocol, it is among the more straightforward ways to conform and provide a grouping of values.
Researched Stack depths [1] ✅, lines 1 ✅, choices [5] ❌, objects 4 ❌
Already the number of stacks to study exceeds the other method.
func perform<T: Decodable>(_ request: RequestProtocol) async throws -> T {
// 3
let data = try await apiManager.perform(request, authToken: "")
}Researched Stack depths [1, 1] ✅, lines 4 ✅, choices [5] ❌, objects 4 ❌
Okay, now we have to choose whether to look in 2 different new places: perform(, APIManager.
Researched Stack depths [1, 1] ✅, lines 4 ✅, choices [5, 2] ❌❌, objects 4 ❌
Already this method has exceeded the other in Objects to learn and the number of choices to be had. Perhaps moving from right to left is an alright way to go. Let’s look at the next perform.
func perform(_ request: RequestProtocol,
authToken: String = "") async throws -> Data {
// 1
let (data, response) = try await urlSession.data(for: request.createURLRequest(authToken: authToken))
// 2
guard let httpResponse = response as? HTTPURLResponse,
httpResponse.statusCode == 200
else {
// 3
throw NetworkError.invalidServerResponse
}
return data
}Researched Stack depths [1, 2] ✅, lines 14 ✅, choices [5, 2] ❌❌, objects 4 ❌
I’m not crazy about a stack being two jumps deep. This method goes a step further. It calls createURLRequest.
// 1
func createURLRequest(authToken: String) throws -> URLRequest {
// 2
var components = URLComponents()
components.scheme = "https"
components.host = host
components.path = path
// 3
if !urlParams.isEmpty {
components.queryItems = urlParams.map {
URLQueryItem(name: $0, value: $1)
}
}
guard let url = components.url
else { throw NetworkError.invalidURL }
// 4
var urlRequest = URLRequest(url: url)
urlRequest.httpMethod = requestType.rawValue
// 5
if !headers.isEmpty {
urlRequest.allHTTPHeaderFields = headers
}
// 6
if addAuthorizationToken {
urlRequest.f
// 8
if !params.isEmpty {
urlRequest.httpBody = try JSONSerialization.data(
withJSONObject: params)
}
return urlRequest
}Researched Stack depths [1, 3] ❌, lines 39 ✅, choices [5, 2] ❌❌, objects 4 ❌
CreateURLRequest doesn’t pass its values in as arguments. It grabs from local properties. However, if you want to jump to them to read them, Xcode will take you to the protocol definitions.
protocol RequestProtocol {
// 1
var path: String { get }
// 2
var headers: [String: String] { get }
var params: [String: Any] { get }
// 3
var urlParams: [String: String?] { get }
// 4
var addAuthorizationToken: Bool { get }
// 5
var requestType: RequestType { get }
}Researched Stack depths [1, 4] ❌, lines 46✅, choices [5, 2] ❌❌, objects 4 ❌
Hmmm… it doesn’t tell us about the values in our instance. It doesn’t have all of them, either. To see the rest, we must use our knowledge about extensions and jump to the extension.
Researched Stack depths [1, 4] ❌, lines 46✅, choices [5, 2] ❌❌, objects 4 ❌
extension RequestProtocol {// 1
var host: String {
APIConstants.host
}
// 2
var addAuthorizationToken: Bool {
true
}// 3
var params: [String: Any] {
[:]
}
var urlParams: [String: String?] {
[:]
}
var headers: [String: String] {
[:]
}
}
A mind can get tired five steps deep. I can see someone staring at these empty values and scratching their head. There is no documentation explaining why there is hard coded empty values. I think it’s better to avoid coding practices that require so much explaining.
We’ve stepped to the bottom of the rabbit hole, but we only have funny values that don’t accurately represent the relevant values. This depth could make it difficult to debug, add a feature, or understand.
We have now exceeded the number of lines of the other version and haven’t studied the whole call stack tree.
Researched Stack depths [1, 5] ❌, lines 55 ❌, choices [5, 2] ❌❌, objects 4 ❌
It takes a bit of research and thought to figure out we have to figure out the relevant types that conform to RequestProtocol. If you jump back up the stack, that’s five jumps. You will have to check each leap to see if you have information about the type conforming to RequestProtocol.
Researched Stack depths [1, 5, 5] ❌, lines 110 ❌, choices [5, 2] ❌❌, objects 4 ❌, figuring things out: 1 ❌
The five layers of the stack conceal the properties of .auth. You have to remember the corresponding auth properties. It requires keeping in mind disjointed information to understand. The median stack depth is 5.
Congrats, we’ve gone through 2 branches of the stack tree. We need to understand Requestmanager, the initializer for Apimanager, and the initializer for DataParser to get a grip on this system.
RequestManager(apiManager: .init(), parser: .init()).perform(AuthTokenRequest.auth)Researched Stack depths [1, 5, 5, 1+, 1+, 1+] +❌, lines 110+ ❌, choices [5, 2, 3, 2, ]+ ❌❌, objects 10+ ❌, figuring things out: 1+ ❌
I can tell you I’m tired of this method enough that I’m just going to hope that my point got through to the reader.
Conclusion:
This article isn’t a knock on Ray Wanderlich and Ray’s website and content. I’ve benefitted significantly, like most of the iOS community has benefitted from the content. My contention is with how engineers make network layers; no one is allowed to question it. I used Ray’s version, partly because RW has a ton of content for any occasion, including this one. Devs know that many devs look to RW for guidance, so I figured I would go to one source.
I find little utility in this method over the more straightforward practices described herein. If there are other points in favor of the Ray method, I would love to hear them and perhaps write a part 2. I’d like to put this to rest.
