12 ms·
I've been developing for a very long time and I'm neither on the side of "lots of comments" or "all code should speak for itself". My philosophy is that commen
by mmastrac 2y ago
I've been developing for a very long time and I'm neither on the side of "lots of comments" or "all code should speak for itself".
My philosophy is that comments should be used for two things: 1) to explain code that is not obvious at first glance, and 2) to explain the rationale or humanitarian reasons behind a bit of code that is understandable, but the reasons for its existence are unclear.
No philosophy is perfect, but I find that it strikes a good balance between maintainability of comment and code pairing and me being able to understand what a file does when I come back to it a year later.
The article is not good IMO. They have a perfect example of a function that could actually make use of further comments, or a refactoring to make this more self-documenting:
function isPasswordValid(password) {
const rules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];
return password.length >= 8 && rules.every((rule) => rule.test(password));
}
Uncommented regular expressions are a code smell. While these are simple, the code could be more empathetic to the reader by adding at least a basic comment:
function isPasswordValid(password) {
// At least one lowercase, one uppercase, one number and one symbol
const rules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];
return password.length >= 8 && rules.every((rule) => rule.test(password));
}
Which would then identify the potentially problematic use of \W (ie: "[^a-zA-Z0-9]"). And even though I've been writing regular expressions for 20+ years, I still stumble a bit on character classes. I'm likely not the only one.
Now you can actually make this function self-documenting and a bit more maintainable with a tiny bit more work:
// Returns either "true" or a string with the failing rule name.
// This return value is kind of awkward.
function isPasswordValid(password) {
// Follow the password guidelines by WebSecuritySpec 2021
const rules = [
[MIN_LENGTH, /.{8,}/],
[AT_LEAST_ONE_LOWERCASE, /[a-z]{1,}/],
[AT_LEAST_ONE_UPPERCASE, /[A-Z]{1,}/],
[AT_LEAST_ONE_NUMBER, /[0-9]{1,}/],
// This will also allow spaces or other weird characters but we decided
// that's an OK tradeoff.
[AT_LEAST_ONE_SYMBOL, /\W{1,}/],
];
for (const [ruleName, regex] of rules) {
if (!regex.test(password)) {
return ruleName;
}
}
return true;
}
You'd probably want to improve the return types of this function if you were actually using in production, but this function at least now has a clear mapping of "unclear code" to "english description" and notes for any bits that are possibly not clear, or are justifications for why this code might technically have some warts.
I'm not saying I'd write this code like this -- there's a lot of other ways to write it as well, with many just as good or better with different tradeoffs.
There are lots of ways to make code more readable, and it's more art than science. Types are a massive improvement and JSDoc is so understandably awkward to us.
Your goal when writing code shouldn't be to solve it in the cleverest way, but rather the clearest way. In some cases, a clever solution with a comment can be the clearest. In other cases, it's better to be verbose so that you or someone else can revisit the code in a year and make changes to it. Having the correct number of comments so that they add clarity to code without having too many that they become easily outdated or are redundant is part of this as well.
- tpmoney 2y ago> Having the correct number of comments so that they add clarity to code without having too many that they become easily outdated or are redundant is part of this as well. In fact, I would argue its not only part of it, it's an essential part. I've worked on projects that were written by folks who were very into the "the source code is the documentation" and comments were extremely minimal. Which was fine for the most part, they did a pretty good job at naming things. The problem was what happened when you were trying to debug a problem. The problems usually start looking like this: "I see ${function} code does X, and that's causing Y which ultimately leads to bug Z, is ${function} supposed to do X, and we should be checking the condition at another level where Y happens or should ${function} not be doing X and that's the root bug?". And no one knows the answer because there's no documentation about what the function is SUPPOSED to do. I view actual comments and documentation about code as the "parity bit" equivalent for CRC checks / RAID 5 storage. If the code and the documentation agree, great now we can have a discussion about whether the code needs to change. If the code and the documentation disagree, then the code is wrong just like if I get data and a parity that don't match, the data is wrong. Your example of the "AT_LEAST_ONE_SYMBOL" with the associated comment is a perfect example of why this is critical. If some months down the line spaces in the password is causing a problem, I now know the code is supposed to be allowing spaces and we know we need to fix the part that's choking on spaces, not the validation rule. Yes it's possible the documentation/parity is wrong, but in either case it calls out a large warning that we're out of sync at this point, and need to get in sync before we can continue.
- LorenPechtel 2y agoArgh! While I agree 100% with your thoughts here I find how you wrote the list of rules to be very hard to read. And I don't like the comment in the middle of the rules, move it above the declaration. Regex is about the opposite of self documenting as it comes, anything non-trivial needs an explanation!
- mmastrac 2y agoWell yeah, that's why I said it was "art" :). It's very subjective what's readable and what isn't, and putting code on HN without syntax highlighting (and with the ironic difficulty of editing code here) is one of hardest ways to communicate.