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.

14 Comments

  • 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. :)

  • 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.

  • 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 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.

  • @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.

Comments have been disabled for this content.