Comment Example Two

Moved and expanded from CommentTheWhy. Created on 2002/12/19, if there are no replies as to how the comment can be avoided and still retain simplicity, I'll take that as strong evidence that the comment is required.

After taking to heart the advice and attitudes found here, my colleagues and I have found that our code has been enormously improved by the removal of comments in favour of more explicit code. Time and time again the code has been modified to make the comments unnecessary, and the code is very much the better for it.

We have now found, however, two cases where we cannot get rid of the comment, and this page is a request for suggestions as to what we might do. The code samples are used in solving a particular set of equations. I can't reproduce them here, they're far too long, but I'll put a small snippet that embodies the problem. Sadly, the code is in a C variant ...

Here's the routine that's called:

    ComplexPair QuadraticSolution(
        complex a,
        complex b,
        complex c
    ) {
        complex discriminant, root, gmean;

        b  = b/2;
        discriminant  = b*b - a*c;
        root   = complexSquareRoot(discriminant);
        gmean  = -(b + signum(b)*root);

        return makeComplexPair( gmean/a , c/gmean );
    }

It's pretty clear the intent, and it's pretty clear the how. What's not at all clear from this code is exactly why we're not using the obvious formula that everyone knows from high-school, that x = (-b +/- sqr(b*b-4*a*c)) / (2*a)

Our example is more complicated than this, and the code we use "obviously" simplifies. The result is that programmers come along, see that it can be simplified, refactor the code to something that they "know" is equivalent but simpler. This usually takes about 30 minutes to an hour. Now they run the UnitTests and fail. Why should it fail? They spend another hour or two, or more, puzzling over why their code isn't the same. The point is that their code is identical, but the machine arithmetic is now unstable.

They're safe - the UnitTests catch the error - but now they have to roll back the changes, possibly not understanding why it didn't work, and leave it alone, puzzled, for the next programmers to fall into the same trap.

We put the comment:

/* Complicated method used to ensure numerical stability */

I would be interested to hear what simpler technique could be used to avoid the hours wasted.

For what it's worth, I think this sort of comment is necessary when it is useful, and anyone who tries to tell you otherwise is hung up on maxims. My taste would be for a longer comment, or at least one that contains a pointer to somewhere where the issue is explained, but YMMV. -- GarethMcCaughan

I think you have a procedural problem, not a programming problem. Why are people randomly jumping into stable sections of code and trying to improve them? I believe all you are trying to convey with your comment is "Don't Change My Code." I have no greater understanding of the problem after reading your comment than before. If they need to go in and make a change, why are they rewriting the entire method before running the UnitTests? If they are refactoring for some good reason, they should be making incremental changes as they go. Once they hit the first change that causes a failure, they can stop their refactoring effort. The time lost should be less than 5 minutes.

People are not randomly jumping into stable code, they are reviewing this code as a critical part of "RefactorMercilessly". The comment is not saying "Don't change my code"; it's saying "This is complex for this reason". In other words, "DontAssumeStupidity".

Refactoring is changing structures not algorithms. If someone is just going in and changing the algorithm because he does not like it, that is not refactoring.

Are you claiming that replacing an algorithm with one that (apparently) has the same result but is more efficient does not come under the heading of refactoring? I find that an astonishing statement, and suspect you are being driven to it by the dogma of avoiding commenting code when it's actually necessary.

Besides, everyone owns the code, so if I come across code using what is clearly an inefficient algorithm I'm allowed to change it. In such cases the comment stands as a warning that the code is as it is for subtle reasons that may not be immediately recognised.

I believe I understand your comments, but in part they don't apply simply because of the specific nature of the real code (rather than this simple example). In the real code, they are attempting to make the semantic components clearer, simply not realising that this code is already as clean and simple as it can be. Furthermore, our environment is against us. Lifting a method is more than five minutes work, and commercial tools don't seem to work in our rather specialised environment.

And finally, why shouldn't I use the comment? Why are people arguing that I shouldn't use a comment? Surely this is an example where it's the SimplestThing.

What does the failing test look like? What is the failing test called? I may be wrong, but a test called 'testWeirdBorderCase()' should definitely call my attention to the problem. Should you ever start a refactoring without at least a quick look through the tests? And even if one doesn't, surely the name of the failed test ('testWeirdBorderCase()', or whatever an appropriate name would be) should give adequate explanation?

Basically, I'm saying you should have one single test devoted to ensuring that the algorithm does what it's supposed to be doing in that special case; be careful that you don't write 'composite' test methods.

-- WilliamUnderwood

This makes sense, although it doesn't entirely solve my problem. I generally believe that looking through the tests should not be regarded as an essential prerequisite for refactorings. You are right that we effectively had (note had) combination tests - we have now separated the question of numerical stability into a suite of tests separate from the "correctness" tests. Still, when we look through code and find "obvious" improvements to make, our general ethos is to make the improvements. The entire purpose of refactorings is to simplify code, not taking every opportunity seems counter to the XP philosophy. A single comment in the code serves to document the potential surprise, and I continue to think it's right. I suspect I agree with GarethMcGaughan?, that refusing to allow such comments is evidence of being hung up on dogma.