How to break down complex if conditions that include optional binding?

Question

I've recently been reading Clean Code and there was a really nice example in java of when it is beneficial to break down a complex if statement condition in to a function rather than use a comment.

// Check to see if the employee is eligible for full benefits
if ((employee.flags && HOURLY_FLAG) && (employee.age > 65))

vs

if (employee.isEligibleForFullBenefits() )

I'd like to be able to utilise this in swift but the nature of optional binding makes this difficult.

if let employeeFlags = employee.flags,
    let hourlyFlag = self.hourlyFlag,
    employee.age > 65 {
        //Do stuff with unwrapped optionals
    }

I can't bundle the condition in to a well named function without force unwrapping as the optionals I've unwrapped are no longer within scope. This is nicer to read but should someone change isEligibleForFullBenefits we could crash when force unwrapping a nil value.

if employee.isEligibleForFullBenefits {
    //Do something with:
    employee.flags!
    hourlyFlags!
}

Another attempt of mine has been to create a function for both the condition and the code block of the if statement:

tryToDoSomethingWithEmpolyeeBenefits() or doSomethingWithEmployeeBenefitsIfElligible()

I avoid force unwrapping but perhaps I'm breaking the single responsibility principle given my function checks for a condition and executes an action based on that condition?

Is one of these a good solution, are there better solutions, or is this simply not a good idea when using swift?

Answer 1

There are a few different approaches we could take to simplify your example code.

Make them not optional (aka null objects and default values)

The most direct way to not worry about unwrapping an optional type would be to not have optional types. If there are sane defaults, we could write accessors that will always return a value and not an optional. Default values are appropriate for simple types (like booleans, ints, etc) where the code is already assuming a default if the optional is not present and the same default value makes sense everywhere we try to use the value. Null objects serve a similar role for more complex types. Null objects implement the same interface as the actual object but "do nothing." Accessors can return sane defaults and command methods can have no-op implementations. If you are using a type of collection (an array, set, dictionary, etc), an empty collection is an effective null object.

If there are no sane default values, you cannot use this strategy. If you need to represent large, complex null objects, maintaining such a large "do nothing" class can be burdensome.

Lambda function parameter

Instead of an employee.isEligibleForFullBenefits method that returns a boolean, we could have an employee.whenEligibleForFullBenefits method that takes a lambda function. The lambda function would take the unwrapped values it needs. It would only be called if those values exist. So the code would look like:

class Employee {
    func whenEligibleForFullBenefits(
            hourlyFlags optionalHourlyFlags: HourlyFlagsType?,
            thenDo: (EmployeeFlagsType, HourlyFlagsType) -> void) {
        if let employeeFlags = self.flags,
            let hourlyFlags = optionalHourlyFlags,
            self.age > 65 {
            thenDo(employeeFlags, hourlyFlags)
        }
    }
}

This makes sense if this is a common condition you have but what you do based on the condition varies widely. The conditional is encapsulated and the behavior is injected.

If instead, this is just a one-off conditional among many, you would quickly find methods like this proliferating with less benefit. This can be the case even if the conditional is the same but the input parameters or the needed values differs. Or if this condition is commonly linked to the same behavior, we wouldn't want to pass it in since we'd be duplicating that logic everywhere.

Tell don't ask

Your example seems awfully interested in the different values inside of employee. Instead of interrogating employee for different pieces of information, tell employee what you want it to do. This gets back to the original point they were making in Clean Code. It also is what you are trying to do by pulling the code into a doSomethingWithEmployeeBenefitsIfEligible() method.

Without seeing your exact code, it is hard to tell if this an ideal solution. If there are lots of different somethings that we could do, we would end up with many methods on Employee, which would become difficult to maintain.

Make it an invariant

You are worried that a change to the isEligibleForFullBenefits() method would cause you to unwrap a nil value. You could declare that the isEligibleForFullBenefits() method returning true implicitly guarantees that the values are not nil as part of its contract. When someone changes the method, they will need to ensure that the invariants still hold. To check this automatically, write thorough automated tests to check the various combinations of states and that your invariant holds correctly. If someone makes a change that violates your invariants, the tests will fail.

"Declaring" an invariant often cannot be checked by a compiler but may instead be checked at runtime, either through tests or through assertions. This means that there is some delay in feedback if someone makes an incorrect change. They could also make a change that is not checked by the automated tests, allowing the invariant to be broken unintentionally. Some invariants cannot be checked by code at all, but can only be specified in comments. Developer diligence is the only safeguard then.