What makes some code confusing?
Developers look at code for hour upon hour every day. After some years of doing this, you can just look at something and almost intuitively understand what it is doing - assuming that some effort has been made by the developers to keep the code clear and understandable. But every now and then, you find a doozy.
I came across this one while working on a feature with Alessandro - my programming pair partner that day.
public bool MayUserChangeEntity()
{
return !(Id > 0 && Security.GetCurrentAccount().IsNormalUser() && Entity.GetEntity(EntityId).EntityNumber > 0);
}
I looked at this with my pair for a good five minutes and was still no closer to grasping it! Why do I struggle to understand this expression?
- Because it is all on one line? No, ternaries are perfectly normal and effective when used well.
- Clearly some time had been spent by the developers to summarize the intent of this method and unfortunately the developers were taking advantage of the && operator to avoid executing the more expensive checks by putting them at the end (this prevented them using Introduce Explaining Variable since the performance gain would be lost).
- Is it the number of terms being evaluated? That seems unlikely since they are not overly complex.
- Is it the negative return? Yes, but more than that. As the number of conditions increase it seems to be harder for me to understand the implication of the negative especially combined with the multiple "and" logic. I think the brain struggles a little with the negation and the boolean && ... you start wondering if all the && become || due to the negation or is that incorrect?
Needless to say, we decided to "refactor to understand" and quickly broke it out into simpler terms:
public bool MayUserChangeEntity()
{
return !IsSaved() || Security.GetCurrentAccount().IsSpecialUser() || IsEntityDraft();
}
private bool IsSaved()
{
return Id > 0;
}
private bool IsEntityDraft()
{
return Entity.GetEntity(EntityId).EntityNumber == 0;
}
Let's review what we achieved:
- We kept the same basic structure of the conditions.
- We created more methods (and more code) but made some of the expressions easier to understand by using a method name to explain the idea. (Using Extract Method instead of Introduce Explaining Variable can be a good way to get clarity but still get the benefits of only evaluating the expression if necessary.)
- We removed the negated boolean logic and converted the conditions to separate positive checks.
It is difficult to say that the resulting code is *MUCH* better but I have no trouble reading the one liner ternary now. The human brain is a strange machine.
We are hiring! Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products?
Take the code test and send your resume and why you want to join Thycotic to tddjobs@thycotic.com.