What makes some code confusing?

re: What makes some code confusing?

Snacks — Mon, 03 Mar 2008 16:45:57 GMT

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.

re: What makes some code confusing?

tahseen — Tue, 26 Feb 2008 10:30:43 GMT

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

re: What makes some code confusing?

Ben — Tue, 26 Feb 2008 09:41:55 GMT

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

re: What makes some code confusing?

Leo K. Scott — Mon, 25 Feb 2008 16:10:08 GMT

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.

re: What makes some code confusing?

thycotic — Mon, 25 Feb 2008 11:46:46 GMT

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

re: What makes some code confusing?

thycotic — Mon, 25 Feb 2008 11:43:29 GMT

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.

re: What makes some code confusing?

Simon — Mon, 25 Feb 2008 08:33:37 GMT

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);

}

re: What makes some code confusing?

Ben — Mon, 25 Feb 2008 07:18:35 GMT

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!

re: What makes some code confusing?

John Bäckstrand — Mon, 25 Feb 2008 05:28:51 GMT

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

re: What makes some code confusing?

tahseen — Mon, 25 Feb 2008 04:54:06 GMT

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.