Bad sign if nobody can comprehend one's code? [duplicate]

Question

If a coder writes code that nobody other than he can understand, and code reviews always end with the reviewer scratching his head or holding their head in their hands, is this a clear sign that the coder is simply not cut-out for professional programming? Would this be enough to warrant a career change? How important is comprehensible code in this industry?

Consider these C examples, compare:

if (b)
  return 42;
else
  return 7;

versus:

return (b * 42) | (~(b - 1) * 7);

Is there ever an excuse to employ the latter example? if so, when and why?

EDIT: Leaving the original latter snippet for consideration, adding a correction:

return (b * 42) | ((b - 1) & 7);

I think the other interesting observation is that it requires that b is 1 for true and 0 for false, any other values would render strange results.

Answer 1

The first rule of any professional software engineer is to write code that is comprehensible. The second example looks like an optimized example for an older, non-optimizing compiler or just someone who happens to want to express themselves with bitwise operators. It's pretty clear what's going on if we are familiar with bitwise operations but unless you're in a domain where that's the case, avoid the second example. I should also point out that the curly braces are missing in the first example.

The coder may insist that his code is efficient, that may be the case but if it can't be maintained, it might as well not be written at all, since it will generate horrendous technical debt down the road.

Answer 2

There are several questions that you raise.

1) Is this a clear sign that the coder is not cut out for professional programming?

• No. Developers often go through stages where they learn about an idea and want to apply it. Do they always apply these ideas efficiently and/or effectively. No. Mistakes are made, and it is part of the learning process. If they are consistently writing incomprehensible code, better communication is required. The reviewers must communicate what the expectations are for acceptable code, and the coder must communicate what the code is (supposed to be) doing and if necessary why it was done in a particular fashion.

2) Would this be enough to warrant a career change?

• Not touching this one.

3) How important is comprehensible code in this industry?

• Extremely. If the code is not comprehensible, you can not be sure of what is doing, nor what it is NOT doing. In other words, you have no way to know if it is working properly.

4a) The C examples:

• The cited C examples are not equivalent. The second case will give the same results as the first if and only if b is restricted to the values of 0 and 1. However, if you throw in a value of say 173,406,926 for b, the results will NOT match.

4b) Is there ever an excuse to employ the latter example?

• Yes. Many processors employ branch prediction and pipelines. The cost of an incorrectly predicted branch may introduce unacceptable delays. To achieve a more deterministic response, bit twiddling might be used to smooth things out.

• My own take on the 2nd example is that it is unnecessarily complicated and should be reworked for clarity. Furthermore, I don't like the multiplication as (depending upon the architecture) it may be slow when compared to other operations. Most likely, I would prefer to see something of the form:

  return b ? 42 : 7;
  

• However, depending upon the circumstance (AND if it could be demonstrated that it provided better substantially better results in critical categories), I might accept a macro with an appropriately descriptive name when reviewing the code. For example:

  /*
   * APPROPRIATE_MACRO_NAME - use (x) if (condition) is 1, (y) if (condition) is 0
   *
   * Parameter Restrictions:
   * (condition) - must be either 1 or 0
   * (x) and (y) are of the same integer type
   *
   * This macro generates code that avoids branching (and thus incorrectly
   * predicted branches).  Its use should be restricted to <such and such>
   * because <why and why>.
   */
  #define APPROPRIATE_MACRO_NAME(condition,x,y)  \
      (((condition - 1) & ((y) - (x))) + (x))
  

Hope this helps.

Answer 3

The second code does not return 42 or 7.

for b = 1:
  (1 * 42) | (~(1 - 1) * 7)
  42 | (~(0) * 7) 
  42 | (-1 * 7) 
  42 | -7
  -5

for b = 0:
  (0 * 42) | (~(0 - 1) * 7)
  0 | (~(-1) * 7) 
  0 | (0 * 7) 
  0 | 0
  0

And yet when you posted it, you thought it did, which is exactly why you should avoid convoluted code.

However, take some "correct" code for example like:

return ((b - 1) & (7 ^ 42)) ^ 42;

There are two reasons I can think of that it may be useful. - The architecture you are writing for does not support branching or predicated instructions. - The architecture you are writing for has a pipeline that either does not work past a branch operation or has a prohibitive cost associated with a branch prediction miss.

In which case you would write something along these lines:

/* 
   This code is equivalent to:

   if (b)
      return 42;
   else
      return 7;

   when b=0 or b=1

   But does not include a branch instruction since a branch prediction
   miss here would cause an unacceptable impact to performance. 
*/

return ((b - 1) & (7 ^ 42)) ^ 42;

If however you just see some code like that with no explanation or rationale, then it's probably a sign of source code obfuscation. Source code obfuscation (as opposed to binary code obfuscation, which many would consider to have legitimate purposes) tends to be nefarious. Some programmers can be territorial for varying reasons, sometimes for job security and sometimes for greater control over what is done and how things are done because of ego, insecurity or distrust. However, it is almost always against the interests of one's peers and one's employer. It is the responsibility of whoever is in charge of the team to stamp out such behaviour sooner rather than later, whether it is by building mutual trust or instilling fear, depending on the management style of the leader and what methods the individual programmer responds to.

Answer 4

Odds are, someone writing (b * 42) | (~(b - 1) * 7) is either someone who knows very little about programming trying to pretend to be experienced/knowledgeable/etc, or is someone trying to sabotage a project (i.e. they're too experienced/knowledgeable/intelligent and want job security).

The first type of person wants to show that they know how to use NOT, OR, order of operations, etc. They're showing off their knowledge, but, alas, they are writing code that's far less efficient, because this requires two multiplications, a subtraction, a not, and an or, which is clearly less efficient than a compare, a couple jumps, and a return.

If that's the case, they don't deserve to be in the industry (yet), but their demonstration proves they know basic computer logic and could be a valuable resource one day, once they get past showboating and start writing efficient code. Also, there's the distinct possibility that b won't necessarily be 0 or 1, which would result in a completely different value being returned. This could introduce subtle bugs that may be hard to find.

The second type of person hopes to slip in code like this (or many other devious types of code), so that people will keep asking them questions about the code, so they are deemed too valuable to lose. This type of sabotage will eventually hurt a project, and they should be let go immediately until they learn their lesson and write optimized, easy-to-read code. There's also the possibility that b won't be 1 or 0, as before, which means it will return a different value than expected (42 or 7), which may work correctly until some hapless programmer changes it back to if(b) ... else ... and the code suddenly stops working. For example, maybe this is a pseudo-number generator disguised as a very expensive if statement.

Legible code is important, as well as code free (as much as practical) from logic bugs such as this one. Anyone that's seriously written code for a while knows how important this is. Write a fully functional game of Tic Tac Toe. Now, set this code aside for a year, then come back to it and try to read the code. If you wrote it offhandedly, without regards for coding standards, commenting, documentation, etc, you would probably not even recognize that the code you wrote was typed by you, much less how to fix it or add a new feature if something was broken or needed to be updated.

When multiple developers work together, it's even more important that code be legible, because odds are, you won't be maintaining that code. You'll have moved on to other things, and someone else will have to maintain your code. Conversely, you may inherit another project and you'll hope that the developers before you left comments and clean code for you to work with. Programmers working on reliable code write the code to be maintainable, including legibility and comments.

Performance, while also important, should not trump legibility. At minimum, if someone did use the second code block here, I'd expect a lengthy comment block that clearly explains why they did it this way instead of a more conventional manner. At that point, I'd probably decide to replace it with more conventional code if there was no good reason for it. If, indeed, it were a logic bomb, I'd re-write it in a longer fashion so it's understood that the function has to be that way to avoid bugs, along with clear documentation of what it really does.

As it turns out, I'm pretty sure that there is a legitimate use for some specialized problem that coincidentally works out to need this precise algorithm. If so, though, I'd expect comments explaining the algorithm that uses this logic, and it had better be for something better than an if-else block. The only two appropriate if-else blocks for the specific example are: if(b) return 42; return 7; (else is optional) and return b? 42: 7; (ternary operators are okay for small branch logic, unless coding standards forbid it entirely). Anything else is overly complicated and should be reduced a simpler statement.

I do occasionally find myself writing "tricky" code that more junior developers may not immediately understand, and I usually comment that code so they can understand why it was written the way it was. Sometimes optimized code can be hard to read, and yet is necessary, but these should be the exception rather than the rule.

There is, coincidentally, a perfectly acceptable place for code like this. Obfuscation contests. In that case, I'd reserve judgement for the function until I determined that the code was just a really clever, CPU-wasting branch, or if it was a more devious device for generating pseudo-random numbers, the value for PI (or some other magical number), or maybe even a weak encryption algorithm.

Answer 5

If the branches in the if/then/else are a problem, then it's probably easiest to just switch to something like:

static const int values[] = {6, 42};

return values[b!=0];

This actually works and although some may find it marginally less readable than the if/then/else, it certainly shouldn't be a noticeable obstruction to anybody who knows C or C++ at all.

For anybody who should think that this is a dirty trick, or depends on particularly loose type checking in one particular language: yes and no. As written the code depends upon "false" converting to 0 and "true" to 1 in C and C++.

The basic idea, however, can be applied equally well in other languages though, including those that have substantially tighter type systems. For example, in Pascal the same basic idea would look like:

var
    values : array [boolean] of Integer;

(* ... *)
values[false] = 6;
values[true] = 42;

(* ... *)
f = values[b<>0];

The Pascal User Manual and Report (2^nd Edition) by Kathleen Jensen and Niklaus Wirth shows the use of a Boolean as an index into an array in a number of places, such as §6.2.1 and §7. Although Pascal (as originally defined) doesn't include the notion of initialized variables, if it did, the initializers would end up in the same order they do in C and C++ (as it does define the fact that false < true).

Ada uses slightly different syntax, but provides the same basic capability:

Temp    : array(Boolean) of Integer := (false => 7, true=>42);

-- ...

Return_Value = Temp( 0 /= i);

So no, we're not dealing with something that's just an accident of one language that happens to use particularly loose type checking. Pascal and (especially) Ada are well known for being particularly strongly typed, yet both support the same basic construct as C and C++, and do so in essentially the same way at that.

Answer 6

If I were presented code like this in a code review, I'd have two questions to pose:

• Why did we elect to write it this way? Since bit manipulation like this is used to circumvent some sort of performance bottleneck, one would presume that we have a bottleneck that is rectified if we employ this approach instead. An immediate follow-up question would be, "How did we prove that this was a critical bottleneck?"

• Do we have any sort of acceptance framework (unit tests, etc) that prove that the optimized approach is equivalent to the non-optimized approach? If we don't, that's when alarm bells go off, but they're not as loud as one might consider.

Ultimately, the code you write should be maintainable. Code that is fast but difficult to broach when a bug occurs in it is not good code. If they could satisfy both of my concerns above, then I'd probably let it go with a request to add both the justification and its equivalent, non-bit form.

In general, a code review should identify and make clear these shortcomings; if there was no justifiable reason to write the code that way, or the code is simply not correct (as a few others have pointed out here), there's no reason to have written it that way in the first place, and it must be corrected. If this is a continuing trend; that is, a developer continues to write code that is efficient but horribly obfuscated, at that point, their lead or senior management should step in and reiterate the importance of clear code.

Answer 7

Something like the following would go a way toward making the INTENT of the code more apparent:

manifoldPressureFloor = (b * 42) | (~(b - 1) * 7);

return manifoldPressureFloor;

manifoldPressureFloor is totally made up, of course I have no clue what the original code is actually about.

But without some kind of explanation or justification for the obfuscated code, the code reviewer and/or maintenance programmer is in the position of having no clear idea what the original programmer really meant to accomplish, which in turn makes it next to impossible to prove that the code actually works (that it actually does what it is SUPPOSED to do). And it makes maintenance programming both painful and much more likely to introduce bugs.

I don't believe (without qualifications) that code can be completely self-commenting. However; I do totally believe it is possible to lean heavily in that direction.

If there is some uncommon (or edge, performance or other heretofore unspecified) reason why that (b * 42) | (~(b - 1) * 7) syntax is justified, the code reviewer wasn't able to easily grasp that fact because the INTENT of the code is not easily deciphered, and there were apparently no comments explaining why that was done.

Code that is clever just for the sake of being clever should be avoided. One of the primary purposes of writing clear code is to ensure human comprehension and proactively prevent bugs from being introduced in the future. Clever code that isn't clearly comprehensible is a time bomb. Even if it works today, it's going to stop working after code maintenance tomorrow, or a year from now. And the bugs will be esoteric and difficult to find and fix.

How can anybody other than the original programmer prove that the code works if the intent and syntax isn't clear? How can even the original programmer prove that the code works in that case?

Comments should be required in screwy, clever edge case code. But its better if the code is simple enough not to require the comments. Those comments also end up out-of-date or irrelevant if the code is updated and the maintenance programmer doesn't update or remove the comments. Programmers routinely update code without updating comments. They shouldn't, they just do.

All of these factors lead to the conclusion that this programmer needs to address the issues of being able to prove that the code works, make the code comprehensible by other human beings, and make the code robust. Code that is highly susceptible to the introduction of new bugs during maintenance is fragile. No employer wants to pay a lot of money for the development of fragile, bug-prone code.

Fragile code, the behavior of which is unprovable, is much more expensive than clean, clear, provable (testable), easy-to-comprehend, easy-to-maintain code. The programmer is being paid by somebody else to be a professional and produce a reasonably clean, robust, maintainable product at a reasonable cost. Right?

So in conclusion, the programmer who came up with that code is obviously smart, and therefore full of potential. But the issues of being able to prove code correctness, make the code robust and comprehensible need to be taken seriously. If that change can take place, the programmer is probably worth keeping on. If not, and the programmer insists on continuing to do things this way, it could become difficult to justify keeping them on the team, because they'll almost certainly cost you more than they'll make you in the long run. And that's the real issue, isn't it?

IMHO.

Answer 8

Replacing if-s with arithmetic/logic expression is sometimes necessary where long processor piping is required. That makes the code to always run the same instructions whatever the condition, making it more suitable for parallelization.

That said, the provided sample is wrong, since the two samples are not equivalent:

if (b)
  return 42;
else
  return 7;

can be return ((b&1)*42)|((1-(b&1)))*7

The programmer of the OP sample may have done a wrong if-elimination, or the OP itself may have misinterpreted the programmer intentions when guessing the if traditional switch in a wrong way. Saying which of the two is correct is hard not knowing what the requirement was.

Note that the expression is not so that "obscure": it' just a linear combination in the form P*(t)+Q*(1-t) (with t=0..1) every programmer should be aware of, since it is the base of most of linear algebra.

The only thing I can understand is that between the coder and the reviewer there is a different cultural base about parallel processing and linear algebra. This IMHO, doesn't make one superior to the other, if correctness is proven.

Answer 9

Besides the excellent points made by Sparky (items 1 and 3) and Donscarletti, this post brings me to another one I answered a long time ago: How do I document my code?.

Lots of people are or call themselves programmers, some are good, not many are excellent. Just like in many other walks of life. You can decide to judge those who appear less good than you are or less good than what you would expect them to be (not very helpful, waste of time), you can try to help them (great) or coerce them to do better (you may have no choice), or ignore them and simply do your best (you may have no choice, this is sometimes the best route). Whatever your choice of action is generally depends on many factors well beyond simple technical ability.