by Leon Rosenshein

Break It Down, Even At Small Scale

domain driven design mmmss code for the maintainer debugging best practices

I’ve talked about taking breaking things down many times. I think it’s a really good idea. I’ve talked about how it applies to commits, pull requests, and projects in general. It can help you move faster and be more responsive to both changing conditions and things you learn along the way.

It also applies at smaller scales. You can have one huge, long method, or you can break the method down into logical parts, with another method around it to manage data flow and decision making. It’s the Unix Way. Each method does one thing, then you have another method that chains them together. It’s easier to understand, easier to modify in the future, easier to debug, and easier to test. What’s not to like?

It applies at even smaller scales than methods. When you have a long computation or an involved condition, there’s always an urge to play code golf and shorten it. Sure, it can work. It might even take a few less characters. But you should resist the urge.

Instead, make the code longer. Just like you should break down long methods into multiple methods that do one logical thing, then use a wrapper method to manage control and data flow, you can break down long calculations. Use well named intermediate variables for storage. Create specific variables to hold the results of comparisons. Then, combine those intermediate variables as needed. If you do that, you’ll find that the result is more readable, more debuggable, and more flexible.

Consider this case I wrote about before.

var filteredEntries []userstate.StateEntry
initialTargetLen := len(targets)
for _, entry := range entries {
    req := entry.Value.(authutils.GetCredentialsRequest)
    // This is getting complicated, so breaking it down. We EXCLUDE a profile if ALL of these conditions are false
    // 1) The user specified a list of profiles and the current profile is one of them
    // 2) The user did not specify a list of profiles AND specified the --all flag OR the --aws flag
    // 3) The user did not specify a list of profiles, the current profile is not an admin profile,
    //       and the user did NOT specify the --admin flag
    // 4) The user did not specify a list of profiles, the current profile is an admin profile,
    //        and the user specified the --admin flag

    if !((initialTargetLen > 0 && targets[entry.Key]) ||
        (initialTargetLen == 0 && (refreshParams.Flags.All || refreshParams.Flags.Aws)) ||
        (initialTargetLen == 0 && roleIsAdmin(req.AWSRole) == refreshParams.Flags.Admin)) {
            continue
        }
    filteredEntries = append(filteredEntries, entry)
    delete(targets, entry.Key)
}

That’s a really complicated conditional, and every comparison is written in low-level terms. A more readable, understandable version might be something like

var filteredEntries []userstate.StateEntry
userSpecifiedList:= len(targets) > 0
refreshableRole := refreshParams.Flags.All || refreshParams.Flags.Aws

for _, entry := range entries {
    req := entry.Value.(authutils.GetCredentialsRequest)
    // This is getting complicated, so breaking it down. We EXCLUDE a profile if ALL of these conditions are false
    // 1) The user specified a list of profiles and the current profile is one of them
    // 2) The user did not specify a list of profiles AND specified the --all flag OR the --aws flag
    // 3) The user did not specify a list of profiles, the current profile is not an admin profile,
    //       and the user did NOT specify the --admin flag
    // 4) The user did not specify a list of profiles, the current profile is an admin profile,
    //        and the user specified the --admin flag
    entryExists := false
    if userSpecifiedList {
        entryExists = targets[entry.Key]
    }
    roleAdminFlagMatchesCLIFlag := roleIsAdmin(req.AWSRole) == refreshParams.Flags.Admin

    if !((  userSpecifiedList && entryExists ) ||
         ( !userSpecifiedList && refreshableRole ) ||
         ( !userSpecifiedList && roleAdminFlagMatchesCLIFlag )) {
            continue
        }
    filteredEntries = append(filteredEntries, entry)
    delete(targets, entry.Key)
}

Overall, it’s more code, but it’s been a looong time since the size of the source code has mattered much. And in the areas that matter to future me, it’s much better. The conditional is much less complex, it’s easier to understand because it reads much more like the English in the comment, and it’s much easier to add to if needed.

I never did make that change, but if I ever do go back into that code that’s the kind of change I’ll make.