<< Iteratees for imperative programmers | Home | Understanding the Play Filter API >>

Comments complement code

Today I read this quote:

Good code is its own best documentation. As you’re about to add a comment, ask yourself, ‘How can I improve the code so that this comment isn’t needed?’

I just want to say, it's a load of rubbish. Take a look at the following code:

def toCharArray(
     decoder: CharsetDecoder = Charset.forName("UTF-8").newDecoder()
  ): Enumeratee[Array[Byte], Array[Char]] = new Enumeratee[Array[Byte],Array[Char]] {

  def step[A](inner: Iteratee[Array[Char], A], partialChar: Option[Array[Byte]] = None)(in: Input[Array[Byte]]): 
      Iteratee[Array[Byte], Iteratee[Array[Char], A]] = {
    in match {
      case EOF => partialChar.map(_ => Error[Array[Byte]]("EOF encountered mid character", EOF))
        .getOrElse(Done[Array[Byte],Iteratee[Array[Char],A]](inner, EOF))

      case Empty => Cont(step(inner, partialChar))

      case El(data) => {
        val charBuffer = CharBuffer.allocate(data.length + 1)
        val byteBuffer = partialChar.map({ leftOver =>
          val buffer = ByteBuffer.allocate(leftOver.length + data.length)
          buffer.mark()
          buffer.put(leftOver).put(data)
          buffer.reset()
          buffer
        }).getOrElse(ByteBuffer.wrap(data))

        decoder.decode(byteBuffer, charBuffer, false)

        val leftOver = if (byteBuffer.limit() > byteBuffer.position()) {
          Some(byteBuffer.array().drop(byteBuffer.position()))
        } else None

        val decoded = charBuffer.array().take(charBuffer.position())
        val input = if (decoded.length == 0) Empty else El(decoded)

        inner.pureFlatFold {
          case Step.Cont(k) => Cont(step(k(input), leftOver))
          case _ => Done(inner, Input.Empty)
        }
      }
    }
  }

  def applyOn[A](inner: Iteratee[Array[Char], A]) = Cont(step(inner))
}

If you know iteratees and you know Scala, it's pretty obvious what this does. It converts a stream of byte arrays into a stream of char arrays, taking into the consideration the possibility that one character may be split across multiple byte arrays. Structurally it is purely functional, however the actual decoding is not, it uses the high performance Java CharBuffer and ByteBuffer classes to do the decoding, which are mutable, and arguably this is necessary since this enumeratee is a place where performance matters. I wrote it, and in my opinion it's not badly written, though if you can see anything that could be improved, please let me know.

So, tell me, on line 14, why do I allocate a char buffer of the incoming byte array length plus one? What is the reason for the plus one? When I first wrote it, I didn't have the plus one there, I didn't think it was needed. You see, when converting an array of bytes to an array of UTF-16 Java characters, at most, 8 bytes will become 8 characters, right? 8 bytes could become 4 characters, if those characters were multi byte characters, the number of chars needed might be less than the number of bytes being decoded, but it can never be more, right? One byte can't become multiple UTF-16 chars, so why would I ever need 9 characters for 8 bytes?

Now maybe you might criticise my code because the +1 is actually a magic number, and if I gave it a name, then that would explain everything. Well, let's give it a name, and reasonable a name (I could give it a two hundred character long name and that might explain everything but you can hardly call two hundred character long variable names good code. Well, maybe you can in Java, but not in Scala). So I'll create a val PotentialMultiCharOffset = 1. Does that help you at all? Do you know what it's for? Why is it 1? Why is it added, why don't I multiply by 2? If you do know the reason behind it, then hats off to you, you are a genius. But for the rest of us, we don't know. It as only after I wrote comprehensive unit tests for the code that I found the bug (I've heard other people say that unit tests are not necessary for functional code, another fallacy).

Let me show you the comment that is above that line of code:

// The +1 here is very important, it is there for the case when there are
// 3 bytes of a 4 byte character in the partialChar array, and so this data
// should contain the final byte, but that one byte will become 2 Chars.
val charBuffer = CharBuffer.allocate(data.length + 1)

Understand it now? Was there any way that I could have written the code that would have explained that? Was there any variable name that I could have given it that would have explained it better than that comment? No, it just needed a simple comment explaining its purpose. Without the comment, you'd be sitting there wondering why on earth I had added 1, you might have even thought "this is allocating more memory than needed, I'll just optimise this" and you would have injected a bug. In this case, a comment is aptly suited to making the code understandable. The comment complements the code. It is necessary and the best way of describing it.

And the fact is that we come across things every day where some really obscure edge case means we have to do some otherwise obscure behaviour. Maybe in a world of higher order logic this isn't the case, but we work in a world of far less than perfect protocols with edge cases that are impossible to memorise, where optimising an equals comparison to return early when you encounter the first character that isn't equal is a security vulnerability, where bugs in other software that our software has to interface to means we have to do counter intuitive things to work around their issues, and where some things are just plain hard to get your heard around, and sometimes a little plain English (or whatever language you speak) just does that little bit to helping you or the next developer make sense of it all.

Comments complement code. Good code does not negate the need of comments. Good code includes comments where comments are needed.



Avatar: Colin Goudie

Re: Comments complement code

 I think you've demonstrated that good code doesn't need comments, and comments are exceptions rather thana rule. So a comment sticks out and that means can be used to highlight exactly what you need in your code above. 

However, if everyline in your code was commented then the imported comments are lost in the noise.

so I would still stand by the quote right at the beginning. Good code doesn't require comments. Nasty or exceptional code should

Avatar: James Roper

Re: Comments complement code

 Well maybe my argument is that exceptional code isn't so exceptional.

Today I wrote a cache that decreased the time certain types of tests in Play framework took to run by 90%.  I used a SoftReference for the cache, and I put a comment there explaining why the reference couldn't be a hard reference, and why a weak reference was also unsuitable.  Yesterday I wrote a hack for shutting down ebean, because it didn't provide any mechanism to do so for the JVM terminated and was leaking resources.  All the code in my hack was completely arbitrary, the only way to know why it was doing what it was doing was to read and fully understand the ebean code and its flow, why should I make the reader do that?  So I wrote heaps of comments to explain so the reader didn't have to do that.  The day before I upgraded Play to Netty 3.5.9, and there was a change where to set a session cookie, you used to have to set the max age to -1, now you have to set it to Integer.MIN_VALUE.  I added a comment to say that Netty semantics for maxAge are that session cookies are to use Integer.MIN_VALUE, so the reader didn't have to dig into the Netty source code to find out why this was.

You can hardly call something that you need to use every day an exception.

Avatar: Colin Goudie

Re: Comments complement code

 I guess I wouldn't measure exception by frequency in a day, rather frequency as compared to the bulk of other code. I just couldn't help but think your original example is a bit of a red herring as it's NOT what most people mean when they say commented code isn't needed. e.g. the examples in Clean Code book. 

Or another way maybe would be to ask, would it be easy for someone to adjust this code WITHOUT adjusting the comment, and if so then the comments ARE BAD. (or a lazy programmer?? But who isn't). When comments become like novels they end up telling lies over time and then you have to revert to reading the code anyway. 

There are no black/white answers here though so they should be judged on a case by case basis, I would just say that for the bulk of code written everyday, most of those cases can be written without needing to add comments.

Avatar: Graham Lea

Re: Comments complement code

Looking at the three examples you give above, and the one in your code, I would have encoded each of these using a test case, not a comment.

Comments don't stop the next guy from deleting the +1, or from completely replacing the code and comment with something they think works better but which also breaks. Executable tests would stop them from doing the wrong thing or prove to them their refactor works.

 

Avatar: James Roper

Re: Comments complement code

I absolutely agree that in the above example, a test is absolutely necessary.  But tests don't help readability of code, you shouldn't have to delete the +1 to find out what it's there for.

In the case where I used a SoftReference, this is basically next to impossible to test, since triggering soft references to be garbage collected is almost impossible to do without accidentally triggering an OOME, and when you do trigger an OOME, there is no guarantee that it will be your code that gets it, it could be another thread in your tests, so you absolutely do not want to do that.

The ebean clean up, you can test for it, but the tests mean nothing, they can only test that it does what I thought it should do, not that they are doing the right thing.  They are also a pain to write and maintain, people are more likely to remove them because they don't understand them then try and maintain them, and so they don't achieve anything.  Comments are much easier to maintain in this case.

Another example where I've put comments is above an equals implementation that compares two strings in constant time, not breaking early if a wrong character is found.  This is impossible to test for, as there is no way to guarantee that a context switch or garbage collection won't happen during the test.  And why is the equals comparison implemented in this way?  Because if it doesn't run in constant time, it's vulnerable to timing attacks.  Another place where a comment is the only sensible thing to use.

Also, tests can be deleted too.  If you delete or ignore arbitrary comments, that's just as bad as deleting tests.

At the end of the day, it's use the right tool for the job.  If tests are the right tool, go for it.  But sometimes comments can go a long way to helping readability, and can describe things that tests can't.

Avatar: Graham Lea

Re: Comments complement code

I think a comment on the +1 is good, though I would make it briefer: "// +1 in case partialChar adds an extra character" at the end of the line and leave it to the reader to inspect the relevant tests if they need to know more.

It's fairly trivial to write code to clear SoftRefereces in a test. I concede the OOME may, in the rare case, not be thrown where you want it, but there are ways to deal with that.

I think referring to comments as a "tool" is misleading. Comments prove nothing and provide no feedback if they go out of date - they only state your hypothesis about the problem and how to solve it, which if you haven't got a test could be wrong. In your SoftReferences case, you've obviously used them because you perceive a problem and think the SoftReference avoids it - and you're probably right - but without a test you haven't proved either that the problem exists (failing test) or that your solution fixes it (passing test). You're just relying on your own thoughts on the day and your comment just says to other people "Trust me, I kno

In brief: Comments are a terrible substitute for tests. If you haven't proved it's broken, why are you writing code to fix it?  :D

[Disclaimer: I've used comments as a substitue for tests perhaps three times in the last 6 years. In retrospect, I did it because I was lazy and wish I hadn't.]

Avatar: James Roper

Re: Comments complement code

Right.  Should I remove every volatile keyword from my code, just because I've never proved that there will be memory visibility issues if it's not there?

And I'd argue that an OOME, in the common case, will not be thrown where I want it.  Most testing frameworks run tests in parallel these days, it's russian roullette as to which threads (yes multiple could, or rather will) get an OOME if you try and trigger one in a test.

I'm not trying to say comments should substitute tests.  Some things are impossible to test, some things are impractical to test, and still other things are useless to test because they prove nothing other than that the code does what it does.  And my reason for making this point is that you said that comments should be replaced with tests.  If you can't test it, you can't replace a comment with a test.

And still, the tests don't help me read the code.  I read hundreds of lines of code of other open source libraries every day.  I'm not reading their tests, I don't even have their tests at hand.  I'm understanding how they work so I can understand how my code should interact with them.  The tests of those frameworks do nothing for me.  They can't help me understand why the framework is doing what it's doing.  But comments do.  My job is made far easier if the source code is well commented.  I can skip over large chunks of code just by reading one comment.

And the argument that comments go out of date is ridiculous.  All documentation goes out of date if it's not kept updated.  Good developers don't let that happen.  Just like good developers make sure they write tests for their code.  It's called discipline.

Avatar: Graham Lea

Re: Comments complement code

> Should I remove every volatile keyword from my code, just because I've never proved that there will be memory visibility issues if it's not there?

> Just like good developers make sure they write tests for their code.  It's called discipline.

Where I work, if I typed 'volatile' in front of a field, I would expect my pair to ask, "Why did you make that volatile?" and no matter how good an explanation I gave, I would expect them to then ask "Can we write a test to prove it?" And we'd do our darndest to write a failing test. Yep, some tests are really, really hard to write. But that's what we do: we write tested code.

Re: Comments complement code

The argument that comments go out of date isn't ridiculous at all, it's just a fact of life. The average programmer... is average. Another fact. So an industry is var better off building up practises that mean that the knowlege is in ONE place only rather than spread out over 3 (code, comments, doc). Of course, this doesn't mean we don't have ANY comments or ANY doc but for the vast majority of code this should be the case.

And again, remember the idea that code should be self commenting, is a practise that lets you ask a question first. e.g. With that code I wrote, I just added a comment. Can extracting a method or a condition to a method remove the need for a comment? yes, then remove the comment. That is what the practise is about, not removing 100% of comments

Avatar: MK

Re: Comments complement code

You are right about that comments complement code. But the quote is not rubbish. It encourages developers to write good, readable code.

Good blog, keep up the good work :)

Avatar: James Roper

Re: Comments complement code

You might be right, I may have been a little over zealous in my criticism there :)  But I think people often use that quote as an excuse for having no comments, and this is what I object to.

Avatar: Ed Dawson

Re: Comments complement code

Great post. There is definitely a case for "always providing the minimul level of human communications" to make something useful to others. You, as the originator, should save your users time by providing that material. 

The old RTFM is being subverted by WTFM "Write The Frigging Manual", used by developers, especially authors of libraries or popular APIs. 

http://www.floopsy.com/post/32453280184/w-t-f-m-write-the-freaking-manual

Avatar: Anonymous

Re: Comments complement code

I totally agree with you and people that say that test should document that are living on another planet.

When one use a thirdparty dependency or even an internal library, one relies on code that supposedly works and where documentation is available on top of the class as well as in the code... When debugging code and stepping down in a library's code, one certainly does not have the test available at end and decipher them. Generally unit tests and functional tests are fairly ugly with time by an order of magnitude compared to the code itself.

Documentation in the test is additional to the one in the code, sometimes expressing a different thing, the documentation in code is there to express some intents for the reader.

It is even more important in Scala. Scala purists seems to often bring the excuse that because it is functional the logic is more apparent. I call BS. Scala code is an order of magnitude less readable due to the complexity of the type system as well as the tendency to chain and compose calls.

<div class="line number8 index7 alt1" style="line-height: 14.296875px; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace; font-size: 13px; border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px 1em !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; min-height: auto !important; white-space: pre !important;"><code class="scala plain" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important;">partialChar.map(</code><code class="scala keyword" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-weight: bold !important; font-size: 1em !important; min-height: auto !important; color: rgb(0, 102, 153) !important;">_</code> <code class="scala keyword" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-weight: bold !important; font-size: 1em !important; min-height: auto !important; color: rgb(0, 102, 153) !important;">=</code><code class="scala plain" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important;">> Error[Array[Byte]](</code><code class="scala string" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important; color: blue !important;">"EOF encountered mid character"</code><code class="scala plain" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important;">, EOF))</code></div> <div class="line number9 index8 alt2" style="line-height: 14.296875px; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace; font-size: 13px; border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px 1em !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; min-height: auto !important; white-space: pre !important;"><code class="scala spaces" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important;">        </code><code class="scala plain" style="border-top-left-radius: 0px !important; border-top-right-radius: 0px !important; border-bottom-right-radius: 0px !important; border-bottom-left-radius: 0px !important; background-image: none !important; border: 0px !important; bottom: auto !important; float: none !important; height: auto !important; left: auto !important; line-height: 1.1em !important; margin: 0px !important; outline: 0px !important; overflow: visible !important; padding: 0px !important; position: static !important; right: auto !important; top: auto !important; vertical-align: baseline !important; width: auto !important; box-sizing: content-box !important; font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace !important; font-size: 1em !important; min-height: auto !important;">.getOrElse(Done[Array[Byte],Iteratee[Array[Char],A]](inner, EOF))</code></div>

is certainly a mouthful in term of types.

As for

<span style="font-family: Consolas, 'Bitstream Vera Sans Mono', 'Courier New', Courier, monospace; font-size: 13px; line-height: 14.296875px; white-space: pre;">Cont(step(inner, partialChar))</span>

it is hard to know what is the intent of it...

Then the case El(data) is many lines long...and it is hard to figure the intent... if it were in Java there would be an effort to factor that at least in one or 2 methods of its own as it would be a few lines longer, here as I mentioned above we take the excuse that Scala allows to write more concise code and we pretend it is more readable, while nothing is further from the truth... just like a diarrhea of documentation does not make documentation, concise code dot not make readable code.

Re: Comments complement code

Well, got to this blog cause I wanted to see Peelbe at work ... but this blog entry is really good to see.   First heard this  'code should be self documenting' BS  from Thougthworks consutant.  

The fact that even a single line of Java (and most more recent languages are far worse),  can take a massive effort to unravel doesn't seem to occur to many.   I don't know how often I've seen dudes sweat on a chunk of code, during my unfortunate 'pair programming' experience,that could of been explaned with a simple comment ... then rage against leaving a comment themselves.  

Another little point .. you can't use code to document what's NOT there.  I regulaly notice this; you find yourself in a situation where to DON'T need to perform an action, yet it may not be ovbious why that is the case ... how else other thatn a comment are you going to make that clear?

Keep up the good work 


Add a comment Send a TrackBack