[jonahx]

I love writeups like this, and enjoyed the level of detail Colin provided.

I take away a different lesson, though: even simple web security is easy to get wrong, even for a very smart, very talented developer. I'm not sure what the solution is, though.

As for the comments, while I don't take a hard line here, I agree with Bob Martin's quote: "Every time you write a comment, you should grimace and feel the failure of your ability of expression." Wherever possible, you are better off rewriting the code and variable names to clarify in the code itself whatever you wanted to say in the comments. It's hard to say how to accomplish that here without knowing more about the code, though. And it may have been so difficult that a comment was the right choice.

[zimpenfish]

Code can only tell you about the implementation - never the intent. Taking an example from my code yesterday:

    $config->{template} = $container->template;

There's not much can be clarified here, I don't think. But it tells you precisely nothing about why it's required in this instance.

(There's no spec for the file format - all have the `template` key in the `container` section but some also have it in the `config` section. Since I can't change these files, I have to deal with the duality. But you'd never be able to guess that from this code without a comment.)

[jonahx]

> Code can only tell you about the implementation - never the intent.

Maybe in some purely technical sense this is true, but in a meaningful one it isn't. At an absolute minimum, names reveal intent -- which is why naming is so important.

Regarding your other example, you are always free to wrap what you don't control in objects that have the intention-revealing semantics (read: correctly named behavior) that you desire.

It's impossible to tell from that single line of code if there are other options as well (the surrounding context is missing, and that's where meaning comes from), but you always have some options. I'll grant that in some cases the cure is worse than the disease -- that is, the changes needed to truly reveal your intent in the code would lead to over-engineered complexity. But typically I find that is not the case.

[MichaelGG]

Naming at the higher level is important (type names). But locals, eh, it's unlikeky that you can comprehend lines without the full context, as you say. And our working memory is limited. So might as well use 1- or 2-char names and keep the code less and thus more easily kept in-head. If this is confusing, there's probably too many locals, so setup new scopes (either by nesting or via separate functions).

[jonahx]

I agree higher level names are far more important.

But I think clarity at the local level is nice, too. Let's say your function takes a name, sanitizes it, and then does some other processing, perhaps storing it. I think this makes the code more immediately clear than 2 character names:

    function storeName(name) {
        safeName = sanitize(name);
        // do other stuff that works with safeName
        //...
    }

[MichaelGG]

In that case it's better to rebind name instead of introducing a new binding - it makes it impossible to misuse it. I hate languages that don't let me rebind (or sometimes, not even shadow).

[zimpenfish]

The surrounding context is "read in this faux-XML file". I could paste the whole method that parses the file but that line would still make no sense without an explanatory comment about the lack of spec (there's about 2x as many lines of comments as code because of this) and that some files have a duplicated `template` key but some don't and all the code which uses the `template` key assumes it's in the `config` section.

[seagreen]

Names are comments:)

  let thisIsAString = 1

But I agree with your overall point.

[jonahx]

This is a great point, actually. And I have seen examples of it in code bases using Hungarian notation.

In practice, though, I think this kind of "lie" both happens and persists less frequently than comments that "lie" either by inaccuracy or obsolescence. People naturally have a lazier attitude toward the upkeep of comments ("they don't really affect the code") and somehow an inaccurate variable name seems more brazen than an incorrect comment.

[strommen]

There are definitely cases where comments are required to describe intent...the "Why?" of the code.

But the problem with comments is that they'll inevitably get out of sync with the code. And a wrong comment is far worse than no comment at all.

In a case like Colin's, I think something as simple as including "secure" or "secret" in the name of the variable would prevent this stuff from happening. If your variable is named `secureAccountCode` then it's unlikely you'll be silly enough to render it back to a hidden input (unless you're writing code comments for "Drunk Me" like the Disqus commenter on the article, in which case all bets are off).

There's a classic Joel on Software article about this, "Making Wrong Code Look Wrong" [1]

[1] http://www.joelonsoftware.com/articles/Wrong.html

[jberryman]

> But the problem with comments is that they'll inevitably get out of sync with the code.

Eek, then by all means take comments seriously and change them when your code changes. I think it's particularly important to comment functions and methods (especially effectful/stateful ones) with what precisely they're expected to do, what invariants hold, etc. (insofar as the type system and function name don't make these clear). Otherwise another programmer has no way (aside from looking at and completely understanding every call site) of knowing what bits of the functionality are incidental vs intended behavior, and are likely to make small modifications for their own purposes which break other things.

Programmers seem very eager to lack of comments and I'm super unsympathetic; this is the one thing that just kills me. Comments hold together a codebase, make it much easier to learn and read, and are a way for programmers to impart all sorts of bits of knowledge about the domain that is valuable "we would do X here but foo() from library Y works in such and such a way so, blah blah blah". And if you don't like to read comments like that I'm sure your editor will fold them for you.

[scott_s]

Agreed with zimpenfish; it's a nice sounding adage, but often we have to do things in code because of non-local reasons. We have to adapt to formats we don't control, or edge cases elsewhere in the system, and there's just no way to embed in the variables and function names that the consumer of this data expects quotes stripped off of strings, so we had better do it here.