Ensure Code Symmetry
| | class BoardComputer { |
| | |
| | CruiseControl cruiseControl; |
| | |
| | void authorize(User user) { |
| | Objects.requireNonNull(user); |
| » | if (user.isUnknown()) { |
| | cruiseControl.logUnauthorizedAccessAttempt(); |
| | } else if (user.isAstronaut()) { |
| | cruiseControl.grantAccess(user); |
| | } else if (user.isCommander()) { |
| | cruiseControl.grantAccess(user); |
| | cruiseControl.grantAdminAccess(user); |
| | } |
| | } |
| | } |
We’ve emphasized a few times already that understandability is absolutely critical for your code. By structuring your conditional branches in a symmetrical way, you’ll make the code easier to understand and easier to grasp. Whoever maintains such code in the future (chances are it’ll be you) will be able to locate features more quickly, and it’ll be easier to spot bugs.
Here is another modification of the code you have seen in Avoid Switch Fallthrough and Always Use Braces. We’ve combined the different conditions into a single one with two else if blocks.
There’s no outright bug in the code. The problem is that all conditions and statements follow each other subsequently. This requires you to read and understand all of them at once. It might not be too difficult here because it’s just a small example without much nesting. But in real life, such structures can grow and become a real puzzle.
In essence, the issue is a lack of code symmetry. Check out this definition by Kent Beck[20]: “Things that are almost the same can be divided into parts that are identical and parts that are clearly different.”
Think about it. Do all branches express a similar concern? Do they show a parallel structure? Or in other words, are all three branches really symmetrical?
The answer is: not really. In the first branch, access is denied. In the second and third branches, access is granted. This isn’t symmetrical.
So how would a symmetrical version of this code look?
| | class BoardComputer { |
| | |
| | CruiseControl cruiseControl; |
| | |
| | void authorize(User user) { |
| | Objects.requireNonNull(user); |
| » | if (user.isUnknown()) { |
| | cruiseControl.logUnauthorizedAccessAttempt(); |
| | return; |
| | } |
| | |
| » | if (user.isAstronaut()) { |
| | cruiseControl.grantAccess(user); |
| | } else if (user.isCommander()) { |
| | cruiseControl.grantAccess(user); |
| | cruiseControl.grantAdminAccess(user); |
| | } |
| | } |
| | } |
The asymmetry in the code came from the fact that we were mixing authorizing code with non-authorizing code. We can improve code symmetry if we separate the two into different blocks of code.
Each part bundles a different type of access with a separate if statement. First, we handle the unauthorized access, log it, and exit the method. Then, we handle the other two cases in an if and a connected else if block.
The second part is symmetrical now, because it contains two types of authorized access. The new structure makes the intent of the condition clearer to the reader. It’s also no longer possible to accidentally introduce the fallthrough bug that we’ve seen in Avoid Switch Fallthrough.
There’s still room for optimization here. For instance, grantAccess() is called in both branches of the second condition with identical arguments. Plus, we could extract the two conditions into separate private methods. This would give us even better signposting in the code.
Note that this points forward to Fail Fast, as we moved the code for unauthorized access to the top.