As you know, I have a thing about conditional code. Most conditionals are duplicates, and the only “genuine” conditionals are at the system boundaries, where they test external state and input information. But I discovered recently that, even at the edges, not every conditional is necessary…
Here’s (a drastically simplified version of) some code I wrote a while back. (It’s from a Rails app, but the message in this post applies to any technology and any kind of system input.)
class Users::RegistrationsController < Devise::RegistrationsController def create user = create_new_user_account if params[:invitation_token] sign_in_as(user) redirect_to Invitation.find(params[:invitation_token]).shared_content else redirect_to new_user_session_path end end end
This route handles incoming user registrations, and it has a conditional branch to cater for the case in which the new user was invited, via email, by an existing user. If the new user arrived by clicking a link in an email we do one set of things; whereas we do a different set of things if the user got here via the ‘Register now’ button on the home page. Perfectly reasonable, I thought. Sure, it’s conditional code; but it’s sitting on the system boundary and therefore it’s fine, yes? No.
This conditional check represents duplication. Somewhere else in my application already knew, at some time in the recent past, which route should be taken through this method when it was finally invoked. When the app sent that email invitation, it was in a sense delegating to the recipient the task of supplying user account details. The conditional branch that handles the invitation is a callback from across a technology boundary, and the inviting code already knew to expect it.
The duplication also has a nasty side-effect: high complexity. This method is an order of magnitude harder to understand and to test(-drive) because of the conditional construct. (And you’re reading a version from which I’ve elided all manner of error handling.) Oh, and by forcing two routes into the same method I also made it violate the Single Responsibility Principle.
Happily the duplication is really easy to remove. The code that sends the email invitation knows which path it wants the registration code to take, so I just need to create a new input route for “invitation acceptance” and change the email invitation to link to it. As a result I’ll have two controller methods, each with simpler logic than their predecessor:
class Users::RegistrationsController < Devise::RegistrationsController def create user = create_new_user_account redirect_to new_user_session_path end end
class InvitationAcceptancesController < Devise::RegistrationsController def create user = create_new_user_account sign_in_as(user) redirect_to Invitation.find(params[:invitation_token]).shared_content end end end
So I have now removed the duplication, and the controller code is simpler as a result. These controller methods are also much easier to test(-drive) than the earlier code. And as a final bonus, I can design the user interface to this route with more knowledge, maybe adding some details of the invitation so that the incoming potential user feels a little more at home. In short, now that each code path has its own method (and class) I can pay it more attention, and it will flourish as a result.
Now, having refactored this code, I take away a few points for the future:
- Where appropriate and possible, model system outputs as delegations into a space that your code can’t reach or control; and model the resulting future input as a response.
- Where a system input is due to the system having earlier delegated a responsibility across its boundary, make sure the return route is differentiated from all others.
- Differentiated incoming routes have code that is easier to read and simpler to test.
- When an input route has a single use, the user experience can be tailored and enriched.
- Not every conditional at the system boundary tests external state.