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.
Comments
18 Responses to “What makes some code confusing?”
Leave a Reply
Jon,
I often write code like this, only later to come back to it and wonder, “What the heck was I doing here?”. Unless I really need the performance gains, I tend to err on the side of readability. In this case I would probably break it out into a couple of If statements. With performance in mind, though, I like what you’ve done. Cool post.
John,
Thanks for posting!
I like to think of each approach to a piece of code as solving the challenge at that time. The idea is we saw this problem and “fixed” it. The next person to approach this code won’t see that problem anymore and will be able to build/improve on ours. This is a very powerful mechanism (I posted about it here: http://weblogs.asp.net/jcogley/archive/2007/03/18/code-review-standing-on-the-shoulders-of-smart-people.aspx )
BTW - when I was putting together this post, I went over to the code to find our refactoring … sure enough, the business requirements had changed (2 weeks has passed since our change) and the method had been reworked by someone else to reflect the new requirements which were very different.
You could argue that the expression was poorly written, but if struggled for five minutes and still couldn’t understand it, I’m glad I’m not paying your salary!
Read up on DeMorgan’s Theorem. Once you grasp that (and it’s pretty simple) expressions like that should not give you any more trouble.
Sam,
Ouch. You must be another bit lover … I have worked with several developers over the years that just lived for bits and bytes.
If a developer has to think about one line of code then that code has failed. I am not saying developers shouldn’t think … just not about one line - I should be able to easily understand any one line in any system. I would sooner be focusing on the requirement I am implementing or the bug I am chasing or general understanding at a slightly higher level. Any significant thought over something small tells me it is too complex.
Thanks for the pointer … from Wikipedia:
not (P and Q) = (not P) or (not Q)
not (P or Q) = (not P) and (not Q)
This makes sense but I think the problem above was more than just that.
I guess the classic argument would be to call any boolean complexity a CodeSmell.
http://c2.com/cgi/wiki?CodeSmell
I agree with Sam. I don’t find the original bad at three arguments, but at more I would have broken it up too. Of course, the two are practically the same so if I stumbled across either I’d be fine with either approach.
I honestly don’t see what the problem is. Being able to do boolean algebra is trivial, part of the job, and you were stumbed by a fairly simple example.
Also, there’s no reason to call someone a “bit lover” because he’s compitent.
Whatever. I guess this is a little reminder why some people stop blogging (one person implies I am not worth my salary and the other implies that I am incompetent or actually in”compitent”).
@thycotic: very good example!
It’s interesting to note that you got some negative comments which implies the problem is pretty wide spread. Programmers do not care about making their code easy to read.
While the refactored code is better, the original is not hard to understand; The method name is good. It’s succint. I wouldn’t have refactored it unless I had a lot of free time on my hands.
John, I think the major improvement in the refactored version of the code is that it expresses the business intend. The IsSaved method hides the implementation detail that id = 0 for new unsaved object and same goes for the other method you introduced.
Sam and Ben, I think all most any reasonable programmer would understand both version and equally everybody will understand the later version faster. Why would somebody waste time jumping through codes to understand (id > 0) means saved? You can say that somebody needs to document that fact which is exactly what the IsSaved method is doing. Self documented code is the best kind of documentation that you can get, cause its never going to be stale.
I would have refactored this in an instant. I also have trouble “seeing” what it does in less than 10 seconds, and thats is enough to know it needs some love.
I have no problem with self-documenting code. As I said, either are fine with me and at over 3 arguments I definately would have made the changes to - 3 is my tolerance limit. I agree 100% with Squid’s remark.
One concern, having seen this happen, is poor refactorings. It may not be true that (id > 0) means saved, which could cause further confusion. When changing code you don’t understand, you need to be extra careful and be due diligent. Eagerly refactoring can be dangerous.
To be honest, my main negativity was (1) the immediate assumption that the original is bad or was done for inept performance, and (2) trying to defame the author of a comment who disagreed with him. While Sam could have been a tad nicer, the blog auther should be the most professional here because he isn’t anonymous. He needs to have a thick skin and take critics openly - developers are a pretty abusive bunch!
I would go further and remove the negation of isSaved() from the ternary
public bool MayUserChangeEntity()
{
return notSaved() || Security.GetCurrentAccount().IsSpecialUser() || IsEntityDraft();
}
private bool notSaved()
{
return !(Id > 0);
}
Ben,
I don’t think calling someone a “bit lover” is defaming someone. Most “bit lovers” that I know are incredibly smart developers who just happen to be comfortable with more bitwise complexity than most developers.
@Simon: We have a team policy of not using negatives in method names - mostly because someone will then do !notSaved which starts to get silly.
I like the way your change reads better though.
The first problem is the name of the method. The calling code “if (MayUserChangeEntry) …” does not read well and it implies that you should be passing in which User to check. A much better name would be ChangeAllowed(). After getting the name right, the next step is to encapsulate the the “magic numbers” into methods that capture the meaning assigned to the numbers. After that either one-line works for me.
Okay, fair enough. Every time I’ve heard that comment stated by a high-level language developer the tone is as an insult. That’s not to say they don’t appreciate it at the right level, but in their programming language it means that the developer is incompetent.
I guess it comes down to it being hard to judge the tone of the written word very well…
@Ben Well said
I wonder when we will have a communication medium as good as a face2fact chat.
Your other point on IsSaved is also true, thats why it may have been better to have the (id > 0) encapsulated behind a method that expresses the intent.
My professional experience is pretty limited and I’m still trying to find the right boundary for breakdown in methods.
This really isn’t an issue. The fact that is confused you so much to the point of writing a blog post probably means that you shouldn’t be a C# MVP. Pay attention to your code. Reading code is like reading a book, one thing at a time.