There is a question I ask myself whenever I’m writing code. Really, it’s become more of a personal mantra. It helps me take a step back and look at my code from the perspective of another developer who might be extending, maintaining or even, dare I say, fixing my software. When mentoring or pair programming, it’s a question that has often led to “aha!” moments for others as well. The question is simple:
Is my code being honest?
Honest code does exactly what it says it will do. No more, no less. Writing honest code isn’t just about meeting the functional requirements. It’s about doing so in such a way that the code’s behavior is clearly expressed and intuitive to any other developer who encounters it down the line. Selfishly, it’s about writing code that’s simple enough for myself to figure out in a few months when I return to it and have forgotten what I was thinking in the first place ?.
If you’ve ever come across of piece of code that did not do what you expected, you’ve encountered dishonest code. Perhaps it was in a third party framework that looked amazing but had unexpected side effects. Perhaps it was when you were tracking down a bug that was caused by a previous “bug fix” which only temporarily masked a larger problem. Or perhaps you encountered a shortcut being taken for convenience, because after all, it worked, and that method was never going to get reused anyway… right?
It’s actually easier this way
The trick to writing honest code is actually pretty simple. Be lazy. Assume that you will have to come back to your own code in the future. Assume that you will have forgotten what you wrote and why you wrote it the way you did. Finally, assume you will be too lazy to read it line by line. Honest code is about looking at what your class names, method signatures, properties and variables are implying, and then making sure that the actual behavior matches what you would assume the code is going to do if you weren’t the author, and couldn’t dig deeper into the source code.
If you write honest code, you’ll have a much easier time when you inevitably come back to it later.
Misleading anti-patterns and their honest alternatives
Over the years there have been a few recurring anti-patterns I’ve noticed that consistently result in misleading, dishonest code. All of the following examples are mocked up. I’m not using real examples from real code.
As you look at the following examples, try to guess what is misleading about them before you read my explanations. How could the code be misinterpreted? How might it be misused, and for what purpose?
Exception Swallowing
In this example, an application is trying to connect to a web API to download a user’s most recent order details. It will use this data to render the user’s most recent order on an account dashboard.
class UserService
def get_most_recent_order(user_id)
user = User.find(user_id)
begin
limit = 1
return OrdersApi.get_by_email(user.email, limit)
rescue HttpError
return nil
end
end
end
What it tried to do: The developer had the foresight to expect service outages from the API. Awesome work! They thought, if the api goes down, we can just return nil, and the dashboard UI that relies on this method will not display any recent order, just like if they were a new user without any orders yet. Seems like a pretty good solution at first glance.
Thinking defensively: Can you think of any other potential use cases for this method? What kind of feature might reuse it? Take a moment now to come up with one or two ideas. Can you be certain that returning nil will still be acceptable in every future use case? Can you be certain that a future developer using your code will read the method body line by line, instead of just looking at the method signature and making assumptions about what it does? Would that future developer expect to receive nil?
How it misleads: 3 months later, another developer is working on a feature elsewhere in the codebase that deletes a user, but only if their last order was fulfilled.
def delete_user(user_id)
order = @user_service.get_most_recent_order(user_id)
if !order.nil? && !order.fulfilled?
raise "can't delete user because their last order hasn't been fulfilled yet."
else
@user_service.delete(user_id)
end
end
Unfortunately, the preceding code won’t work. If the API goes down, the code will delete users who shouldn’t get deleted. Why? Because the order history is nil. The get_most_recent_order method might tell lies. The most recent order isn’t really nil, it’s just inaccessible at the moment. Those are two very different things.
Worst of all, it probably worked just fine during custom software development and testing, when there were no API outages. This bug might not be discovered until it’s already been in production for months or even years, increasing the impact of the error, as well as the difficulty of finding and resolving the bug.
The honest version: Don’t hide exceptions. Exceptions are meaningful information and might need to be handled differently depending on who called the method. Let exceptions bubble up to the last layer that can handle them. Let the UI layer decide to handle the exception and leave the area of the dashboard blank, gracefully hiding the error from the user. Let the delete account feature choose to handle the error differently, perhaps showing an error message to the administrator. The highest level of code should at the very least be aware that errors occurred, so it can make it’s own choices about how to handle them.
Further reading: Swallowing exceptions is bad for your health
Single Responsibility Violations
In the next example, someone has implemented authentication with an OpenAuth provider like Google. When a user clicks “login with Google”, the application will receive an email address. The following method is used to get the user account associated with that email.
class UserService
def get_user_by_email(email)
user = User.find_by(email: email)
if user.nil?
user = User.new(email: email)
user.save
end
return user
end
end
What it tried to do: This seems like a pretty reasonable shortcut. The developer thought, hey, if the user doesn’t have an account yet, and they are trying to login with their Google account anyway, let’s just automatically make them an account. Great feature idea. Questionable implementation.
Thinking defensively: Once again, take a moment to come up with a few ideas for how this method might get reused. When else might someone be interested in finding a user associated with an email? Would all of those cases benefit from automatic user creation? If you only looked at the method name, would you expect this behavior?
How it misleads: The method is called get_user_by_email. How can you get something that doesn’t exist yet? Are you really “getting” a user when you create a new one? 6 months later, another developer is adding validations to the old-fashioned manual sign-up page.
def email_is_in_use?(email) found_user = @user_service.get_user_by_email(email) return !found_user.nil? end
It looks like such a simple validation. Obviously it will work. Business deadlines are looming and pressure to deliver is high, so the developer checks this validation in without testing and it goes to production. Soon after, customer service gets swamped with angry potential users who are being told their email addresses are already in use. Why? The get_user_by_email method lied.
It didn’t only get users, it created them too. It does more than one thing, both fetching users if they exist, and also creating users if they don’t. Violating the single responsibility principle often leads to misleading method names like this. Sure, proper unit testing would have discovered the error. But we’ve all seen codebases that lacked test coverage. This mistake would have been avoidable from the start if the code was being honest.
The honest version: Similar to the exception swallowing problem, the solution is often to move this code up a layer or two. Let the application controller decide that it wants to create a user if one doesn’t exist. If the controller or some other higher level code wants to automatically create a user, it can do so by calling a separate method which follows the single responsibility principle by only creating a user.
class UserService
def get_user_by_email(email)
User.find_by(email: email)
end
end
class OAuthController
def on_postback(oauth_payload)
email = payload[:email]
@user = get_user_by_email(email)
if @user.nil?
@user = User.new(email: email)
end
end
end
At first glance, this code is doing the same thing. The difference is who is doing it. The OAuthController is deciding to create a new user because a user didn’t exist. As opposed to getting a new user without even knowing they are new. We’ve offloaded the decision to a higher-level component where it belongs, and given the high-level component all the information it needs.
Further reading: SOLID Part 1: The Single Responsibility Principle
Hiding Side Effects
In the next example, we have a javascript framework for publishing a custom event and registering a callback method that listens for the event.
App.Dispatcher = {
events = {},
on: function(eventName, callback){
App.Dispatcher.events[eventName] = callback;
},
trigger: function(eventName){
var callback = App.Dispatcher.events[eventName];
if(typeof(callback) === "function"){
callback();
}
}
}
What it tried to do: At the time that this was written, all seemed fine. It served it’s purpose of allowing a developer to register a callback and trigger an event. Influenced by popular frameworks like jQuery, the developer probably decided to copy naming conventions by using the method names on and trigger. Which correspond to jQuery’s methods for registering and triggering DOM events on DOM elements.
Thinking defensively: If you saw the on method being used in various places throughout the codebase, what assumptions would you make about how it works under the hood? What would happen if someone registered more than one callback for the same custom event? Would it handle multiple callbacks in a similar way to jQuery’s on method? Would you expect it to?
How it misleads: The problem here is subtle. It requires thinking about how the code might be used, not just how it is going to be used for the initial use case. 1 year later, a developer sees this code in use, and decides they can use it for similar purposes. They want to coordinate a few different UI components so that they each re-render themselves when a change occurs in the data.
// in account-summary.js
function AccountSummary() {
var self = this;
App.Dispatcher.on("account-balance-changed", function(){
self.render();
});
};
// in savings-chart.js
function SavingsChart() {
var self = this;
App.Dispatcher.on("account-balance-changed", function(){
self.render();
});
};
At first glance, I would totally expect this code to work. It looks like we have hooked 2 callback functions up to the “account-balance-changed” event. Simple enough, right? Except that the AccountSummary component will never update itself. Why? because the on method deregistered the AccountSummary callback when the SavingsChart callback was registered.
Take a closer look at the original method. It uses a simple hash to keep one callback function associated to each event. When you register a second event handler it overwrites the first.
The honest version: If the original developer wanted to restrict the framework to only allow one callback per event, that’s fine. They just didn’t tell us. We can give them the benefit of the doubt and say that maybe the documentation mentioned it, but we’re being lazy, remember? We want our code to imply exactly what it’s going to do, not force us to go sifting through the source or the documentation, wasting valuable time.
Here, I would do two things. First, rename the method so that it doesn’t resemble jQuery’s similar method (and thus inherit developers’ preconceived notions of how it should work). Second, explicitly disallow multiple registrations, so that if a developer misuses it, they find out exactly what went wrong, very quickly.
App.Dispatcher = {
events = {},
register: function(eventName, callback){
var existingCb = App.Dispatcher.events[eventName];
if (existingCb != null) {
throw "Not allowed to register more than one callback for event => " + eventName;
}
App.Dispatcher.events[eventName] = callback;
},
trigger: function(eventName){
var callback = App.Dispatcher.events[eventName];
if(typeof(callback) === "function"){
callback();
}
}
}
Of course, the original developer could have just allowed more than one callback to be registered by storing them in an array. But they didn’t need multiple registrations at the time, so they didn’t over-engineer the thing by adding features that might be used. Good call. If you don’t need the extra functionality, don’t build it until it becomes needed. However, you can still anticipate how a future developer might interpret and use you code, and write defensively to prevent misuse, as in the honest example above.
Further reading: Custom Exceptions: When should you create them?
Now be honest, how’s your code looking?
If you haven’t yet learned about good software design principles such as the SOLID principles, I highly recommend you start researching them. They will help embed ideas in the back of your mind that influence your coding decisions greatly. However, I have found that for many people, myself included, they stay in the back of the mind. It’s impractical to expect that anyone will consciously walk down the list of design principles, checking them off one-by-one for each line of code they write.
For me, it’s much more helpful to have a few core values that I keep up front. The core values help me identify bad code smells. Then I can then refer to design principles to figure out where the smell is coming from. The most important thing, however, is identifying that something is wrong in the first place. Honest code is a value that helps you identify these problems early.
Try this mantra on for a while. Every time you commit code for the next week, ask yourself, “is my code being honest?” Go through your changes and review each item under this new lens. You may be surprised at how many positive changes you can come up with, and, pretty soon, it will become second nature.
