Don’t Use “if” Blocks

I want to show you a bit of pseudo code. This is what I see at work in some of our code, and also looks a bit like code I’ve seen elsewhere. This adds items to a menu. The actual items is not relevant so I skip showing the item details:

if( isAdmin ) {
    AddMenuItem( ... );
}
AddMenuItem( ... );
AddMenuItem( ... );
if( isAdmin ) {
    AddMenuItem( ... );
}
if( isSuperAdmin ) {
    AddMenuItem( ... );
    AddMenuItem( ... );
} else {
    AddMenuItem( ... );
}
AddMenuItem( ... );
AddMenuItem( ... );
if( isRegionalAdmin  || isSuperAdmin || isAdmin ) {
    AddMenuItem( ... );
}
AddMenuItem( ... );
if( isAdmin ) {
    AddMenuItem( ... );
}

I find myself writing code like this now and then, but I usually change it drastically once there are more than two conditional blocks. Sometimes I don’t even let it get that far before “fixing” it. The thing is, I don’t like reading source code and mentally building a picture of the result from a bunch of tests and blocks. What I really want to see, in this case, is what the menu items look like. Code like this may be clear to some people, but it doesn’t let me quickly scan it and get an overview of what’s going on.

The fix is to write a small function that accepts information about the current user and a set of flags to test in that function. The menu code ends up looking more like this:

AddMenuItem( ..., userInfo, isAdmin );
AddMenuItem( ..., userInfo, everyone );
AddMenuItem( ..., userInfo, everyone );
AddMenuItem( ..., userInfo, isAdmin );
AddMenuItem( ..., userInfo, isSuperAdmin );
AddMenuItem( ..., userInfo, isSuperAdmin );
AddMenuItem( ..., userInfo, ~isSuperAdmin );
AddMenuItem( ..., userInfo, everyone );
AddMenuItem( ..., userInfo, everyone );
AddMenuItem( ..., userInfo, isRegionalAdmin  | isSuperAdmin | isAdmin );
AddMenuItem( ..., userInfo, everyone );
AddMenuItem( ..., userInfo, isAdmin );

Some programmers might think this is a bit heavyweight, and maybe it is. But I don’t need to see all those “if” statements in this code; they are not relevant and are, frankly, distracting.

This is pseudo code and I didn’t give a lot of thought to the mechanism I would use for the actual function arguments. I would have passed in the results of an actual bitwise operation like this:

AddMenuItem( ..., userType & ( isRegionalAdmin | isSuperAdmin | isAdmin ) );

Or in swift, I could use an OptionSet:

AddMenuItem( ..., userInfo, [.isRegionalAdmin, .isSuperAdmin, .isAdmin] )

And knowing me, I would spend 20 minutes figuring out how to make the code as “elegant” as possible. I don’t now what is objectively attractive code, but I know that those “if” tests look terrible.

There are still issues in this code. For instance, how do I tell the function to skip the menu item for a type of user? The solution to this problem isn’t important; what matters is that the clutter has been removed.