How not to write shitty code: Intentional Conditions

I think this is probably going to be a series. I’ve read enough shitty code (including my own and that of coworkers I have a lot of respect for) that I think I can offer a few tips on how not to write shitty code. This is going to be the first, which I call Intentional Conditions.

When you’re writing a condition, you should think carefully about not only what you have to check but why you’re checking it. What is the meaning behind the check? How would a human check this? Often, a condition has prerequisites, a primary rule and a list of exceptions. The expression you came up with to express this might not match how you (or anyone else) thinks of it!

For example, let’s say we wanted to save a person record. You might need to know the following:

That some value of the record has actually changed.
That the record is not a built-in that can never be changed.
That the user has permission to change the record.

The code might look like this (and I should warn you, I’m not even going to try to write this in a real language, let alone compile it):

if (!record.isBuiltIn() && (!record.firstName.equals(oldRecord.firstName) || !record.lastName.equals(oldRecord.lastName) || !record.company.equals(oldRecord.company) || !record.department.equals(oldRecord.department)) && user.canChange(record)) {
    // can't save
}

First, stop and break it into a function.

if (!canSave(record, old_record) {
    // you can't save this record
}

boolean canSave(record, old_record) {
    return !record.isBuiltIn() && !(record.firstName.equals(oldRecord.firstName) || !record.lastName.equals(oldRecord.lastName) || !record.company.equals(oldRecord.company) || !record.department.equals(oldRecord.department)) && user.canChange(record);
}

You’ve already improved the condition because you’ve given it a name.

Now, think about the rule and the exception. The rule here, I think, is that the record has changed. The exceptions are the reasons you can’t save it.

boolean canSave(record, old_record) {
    boolean hasChanged = (!record.firstName().equals(oldRecord.firstName())
        || (!record.lastName().equals(oldRecord.lastName())
        || (!record.company().equals(company.firstName())
        || (!department().equals(department()));
    return hasChanged && !record.isBuiltIn() && user.canChange(record);
}

But that’s still going to be a massive pain to debug. So let’s use the “return early, return often” style to make this more readable:

boolean canSave(record, old_record) {
    boolean hasChanged = (!record.firstName().equals(oldRecord.firstName())
        || (!record.lastName().equals(oldRecord.lastName())
        || (!record.company().equals(company.firstName())
        || (!department().equals(department()));
    if (!hasChanged) {
        return false;
    }

    if (record.isBuiltIn() {
        return false;
    }

    if (!user.canChange(record)) {
        return false;
    }

    return true;
}

Some would argue that the last two returns should be combined, but I think there’s not much difference between returning frequently and relying on short circuit boolean expressions. What matters is how easy it is to debug, and it’s much easier to put breakpoints on unexpected returns if they’re one per line.

You’ll also notice I put hasChanged into a temporary variable. This is 2020, and even if your compiler/optimizer isn’t smart enough to produce equally efficient code, unless you’re sure this is a bottleneck you can spare the cycle or two to handle that. Naming things helps understand them.

There’s a whole argument to be had regarding keeping function results in local variables for easier debugging, but we’ve mostly sidestepped it here. You’ll be able to tell if the record is built in based on what you hit when you step, for example.

Now remember how I said we should think about the prerequisites, primary rule and the exceptions? Now that we’ve written it out this way, I think we’re checking things in the wrong order. It’s probably more important that function fail early if the user doesn’t have the ability to change it. These are more prerequisites than exceptions.

So let’s tweak that:

boolean canSave(record, old_record) {
    if (record.isBuiltIn() {
        return false;
    }

    if (!user.canChange(record)) {
        return false;
    }

    boolean hasChanged = (!record.firstName().equals(oldRecord.firstName())
        || (!record.lastName().equals(oldRecord.lastName())
        || (!record.company().equals(company.firstName())
        || (!department().equals(department()));
    if (!hasChanged) {
        return false;
    }

    return true;
}

There’s still room for improvement: that last if is hard to read, after all. How could we make it better? The next step would is to split off hasChanged as its own function, providing a quick way to compare if two records are equivalent. Then we could drop the temporary variable if we liked.

If we did that, we’d end up with a canSave function like this:

boolean canSave(record, old_record) {
     if (record.isBuiltIn() {
         return false;
     }

    if (!user.canChange(record)) {
        return false;
    }

    if (record.valuesEqual(oldRecord)) {
        return false;
    }

    return true;
}

Not everyone will love this code, but it’s very straightforward to read.

You’ll breathe a sigh of relief years from now when you need to debug this! Whether it’s optimal or not, it’s less shitty than the original.