Should a transformation function take a nullable and return a nullable or should the caller handle nullability

Question

Often I need to transform a type to another, such as a networking model to a data model, or a data model to a binary representation. Should these transformation functions take an Optional/nullable value and immediately return nil if it's nil, or should the function only accept non-nil values and always return a non-nil transformation? Assume my code properly handles validation when receiving data, so no further validation is needed, just transformation.

Example 1: Have the caller handle nil objects

func makeUser(from apiUser: APIUser) -> User {
    return User(id: apiUser.id)
}

var user: User? = nil
func downloadUser() {
    service.getUser { (apiUser: APIUser?) in
        if let apiUser = apiUser {
            self.user = makeUser(from: apiUser)
        } else {
            self.user = nil
        }
    }
}

Example 2: Have the transform handle nil objects

func makeUser(from apiUser: APIUser?) -> User? {
    if apiUser == nil {
        return nil
    }
    return User(id: apiUser.id)
}

var user: User? = nil
func downloadUser() {
    service.getUser { (apiUser: APIUser?) in
        self.user = makeUser(from: apiUser)
    }
}

Example 1 explicitly states an APIUser will always transform to a User object. While Example 2 implicitly implies that a valid APIUser may not map to a valid User object, even though that is guaranteed.

While Example 2 does demonstrate that the common logic is handled in the transform instead of copied each time it's needed, I personally don't like the fact that the transform can return an optional/nullable value even though my function contract states validation has already been performed and a valid value will never fail.

Answer 1

If the only purpose of an optional parameter is to return nil if the parameter is nil then I wouldn’t do it. Get some additional type safety by making the parameter non-optional. Then anyone having a possibly nil value can check for that. Quite often it turns out the value is actually never nil and there is no checking code in the end.

Answer 2

The makeUser function takes a value and creates a user out of it. If it cannot create a user because the input is invalid, it should throw an exception. Null is definitely an invalid input as you can't parse "nothing" and get a User.

This is similar to parsing an int. If the input is invalid, throw an exception, don't pretend it went well and then return the default value.

If you do this, then whenever you call makeUser you know that you definitely have an actual User object, not null.

downloadUser should then validate that the user it downloaded is valid (in your case that seems to just be checking that it isn't null).

While it doesn't particularly matter for this one use case, the benefit is that other calls to makeUser either work (in which case you have a user object) or have to explicitly be handled by the calling code. This helps reduce the chances of null reference exceptions throughout the app.

EDIT: For summary, Example 1 is better to me.

Answer 3

The APIUser, being a DTO, exists at the boundary of your application. It should not be leaking out beyond the object that interacts with the remote service. Neither example is the right solution. The real problem lies with your service not returning a User object and performing the mapping from APIUser to User internally.

As for nullability? It's a remote system. You do not control it. It can be null.

Your service should be refactored to return a User?. If the API returns a null value, the service should return a nil. You need to give your application a chance to handle situations where the user does not exist in the remote service. This might be an innocent mistake where someone tried to log in, but they don't have a user profile yet. No need to crash the application when a polite "You do not have permission to access this application" message would suffice.

So basically the answer is Example 3 (below)

// 'service' calls API and maps to User domain object
var user: User? = service.getUser()

if user == nil {
    // user not found in remote service, handle error
}

This is honestly no different than querying a database for your application. The user might not exist. Return a nil instead of crashing the application with an exception you probably need the user to handle anyhow.

Answer 4

Callers might want to handle nil differently

The power of Optional is that it gives users to handle nil in many different ways.

1. They can substitute a default value using the nil-coalescence operator (??)
2. They can assert that it's not nil, using the force unwrap operator (!)
3. They can transform the value, using map or flatMap

There's no universally correct approach. So any determination you make within your callee-side handling is likely to be wrong for at least some callers. That's why we should let callers handle nil themselves.

It would make a total mess

Thinking about this practically, if it was decided that "callees should handle nil for callers", than all Swift code would be pervasively littered with optionals. That's not very great.

There's already a tool for this job

What you would find is a lot of repetition of the same code that you just wrote "check if x is nil, and if it's not, transform it by passing it through f, otherwise just pass the nil along quietly". When we identify repetition in code like this, we should aim to extract it out using some kind of abstraction. In this case, it already exists, in the form of Optional.map(_:).

Given these two functions:

func makeUserHandlingOptionals(from apiUser: APIUser?) -> User? { ... }
func makeUser(from apiUser: APIUser) -> User { ... }

One can be trivially implemented in terms of the other:

func makeUserHandlingOptionals(from apiUser: APIUser?) -> User? {
   return apiUser.map(makeUser(from:))
}

It's so trivial, in fact, that you shouldn't declare makeUserHandlingOptionals(from:) at all. If a caller wants to transform a APIUser?, just let them call map themselves.

A similar story in Java

In the past, I've had a similar temptation occur to me. I was writing Java before 1.8 (which introduced the stream APIs). I had a function that took an A and returned a B. But I had an ArrayList<A>, and it was very tedious to convert it to an ArrayList<B>. I had to do it myself every time:

B convertAtoB(A a) { ... }

ArrayList<A> inputOfAs = ...
ArrayList<B> outputOfBs = new ArrayList();

for (A inputA : inputOfAs) {
    outputOfBs.add(convertAtoB(inputA));
}

I needed to do that a lot, and that was annoying. I've had a similar temptation as you did, perhaps I should introduce a second overload version of convertAtoB, which took an ArrayList<A> and returned an ArrayList<B>. But then that seems annoying, so maybe the right choice is to only have the array version, and pass it single-element arraylists when I need to transform a single element.

I didn't have a good solution, until I eventually learned about streams and "functional style" constructs like map. map completely solves this problem, by abstracting away the details of making an (A) -> B function able to work as if it were a ([A]) -> [B] function. That way, convertAtoB can stay focused on converting A to B, and not fiddle around with array lists.

Switching to map even improved performance. At the time, I didn't realize that repeaded ArrayList.add calls would cause occasional resizing operations. When the current array got full, ArrayList would have to allocate a new larger one, and spend O(n) time copying the old elements over. Only to do so again momentarily later. The implementer of map would know that the input and output arrays always have the same size (by the definition of the map). Thus they could pre-allocate an output array that's precisely large enough to fit all the new B elements, without resizing and without excess. That's something I hadn't thought about at the time, and got "for free" in the process.

Conclusion

TL;DR: Your function should stay focused on doing the transformations it's responsible for.