[HN Gopher] Self-Documenting Code
___________________________________________________________________
Self-Documenting Code
Author : tie-in
Score : 37 points
Date : 2024-10-23 13:33 UTC (1 days ago)
(HTM) web link (lackofimagination.org)
(TXT) w3m dump (lackofimagination.org)
| joecarrot wrote:
| If one of my developers used "||" that way I would definitely
| throw some side eye
| JellyBeanThief wrote:
| I was thinking exactly the same. You can write
| if (cond) { cons }
|
| on one line and get more readable code admittedly a few chars
| longer.
| alilleybrinker wrote:
| Code patterns are social! What is strange to one is normal to
| another.
|
| The kind of pattern used here with the `||` might seem weird
| to some JavaScript developers, but it's pretty normal in
| shell scripts, and it's pretty normal in Ruby with `unless`!
| mmastrac wrote:
| `||` is conventional in bash, but definitely not in JS. `x
| || value` is closer to conventional JS, but using it for
| control flow is certainly not common and a stylistic
| choice, for sure.
| 65 wrote:
| Don't even need the curly braces. I do if
| (cond) doSomething();
|
| all the time.
| gnarlouse wrote:
| if (heathen) dontUseCurlies();
| Groxx wrote:
| if (dontUseCurlies): goto fail goto
| fail
| gnarlouse wrote:
| go to jail
| MetaWhirledPeas wrote:
| I must be their target audience because as soon as they used
| the example with || it all started making sense.
|
| This would have been fine too but it would trigger some people
| not to use {} if (!validateUserInput(user))
| throwError(err.userValidationFailed);
|
| My preferred style might be closer to this.
| if (!userInputIsValid(user))
| throwError(err.userValidationFailed);
| gnarlouse wrote:
| If one of my developers threw in a throw like that I would
| throw up in their mouth.
| gspencley wrote:
| I agree with most of the article but want to nitpick this last
| part:
|
| > I'm not a fan of TypeScript, but I appreciate its ability to
| perform static type checks. Fortunately, there's a way to add
| static type checking to JavaScript using only JSDoc comments.
|
| If you're writing JSDoc comments, then you're not writing what
| the author considers to be "self-documenting code."
|
| I wish the author had explained _why_ they are not a fan of
| TypeScript. Compile time type-safety aside, as the author
| acknowledges by implication adding type specificity negates the
| usefulness of JSDoc comments for this particular situation.
|
| I'm personally a big proponent of "self documenting code" but I
| usually word it as "code that serves as its own documentation
| because it reads clearly."
|
| Beyond "I would personally use TypeScript to solve that problem",
| my case for why ALL comments are a code smell (including JSDoc
| comments, and in my personal opinion) is:
|
| - They are part of your code, and so they need to be maintained
| just like the rest of your code
|
| - But ... they are "psychologically invisible" to the majority of
| developers. Our IDEs tend to gray them out by default etc. No one
| reads them.
|
| - Therefore, comments can become out of sync with the code quite
| easily.
|
| - Comments are often used to explain what confusing code does.
| Which means that instead of fixing the code to add clarity, they
| do nothing but shine a spotlight on the fact that the code is
| confusing.
|
| - In doing the above, they make messy code even messier.
|
| I am slightly amenable to the idea that a good comment is one
| that explains WHY weird code is weird. Even then, if you have the
| luxury of writing greenfield code, and you still need to do
| something un-intuitive or weird for really good reasons ... you
| can still write code that explains the "why" through good naming
| and separation of concerns.
|
| The only time that I would concede that a code comment was the
| best way to go about things in context is when you're working
| with a very large, legacy commercial code-base that is plagued by
| existing tech debt and you have no good options other than to do
| your weird thing inline and explain why for logistical and
| business reasons. Maybe the refactor would be way too risky and
| the system is not under test, the business has its objectives and
| there's just no way that you can reasonably refactor in time etc.
| This happens... but professional developers should ideally treat
| incremental refactoring as a routine part of the development
| lifecycle so that this situation is as unlikely as possible to
| arise in the future.
| mjlawson wrote:
| I find myself agreeing with much of your point, but I feel the
| need to nitpick a bit of your comment myself :)
|
| I don't think your code base needs to be very large, or very
| legacy in order for comments to be valuable or even the best
| way forward. If the decision exists between a somewhat large
| refactor or a one-off comment to account for an edge case, I'm
| likely to take the latter approach every time. Refactors
| introduce risk, add time, and can easily introduce accidental
| complexity (ie: an overengineered solution). Now once that edge
| case becomes more common, or if you find yourself adding
| different permutations, yeah I agree that an incremental
| refactor is probably warranted.
|
| That said, perhaps that comment could -- and certainly one
| should at least supplement it -- be replaced with a unit test,
| but I don't think its presence harms anything.
| mmastrac wrote:
| 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 wrote:
| > 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 wrote:
| Argh! 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 wrote:
| Well 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.
| alilleybrinker wrote:
| There is no such thing as universally self-documenting code,
| because self-documentation relies on an assumption of an audience
| -- what that audience knows, what patterns are comfortable for
| them -- that does not exist in general.
|
| Self-documenting code can work in a single team, particularly a
| small team with strong norms and shared knowledge. Over time as
| that team drifts, the shared knowledge will weaken, and the
| "self-documenting" code will no longer be self-documenting to the
| new team members.
| dvt wrote:
| The writer here misunderstands how short-circuit evaluation is
| supposed to be used. The idea is that you should use SCE in a
| few, pretty standard, cases: cheapFunction(...)
| || expensiveFunction(...) // saves us a few cylces car =
| car || "bmw" // setting default values, common pattern
| funcA(...) && funcB_WhichMightBreakWithoutFuncA(...) // func A
| implies func B ... // probably a few other cases
| I don't remember
|
| Using it to handle control flow (e.g. throwing exceptions, as a
| makeshift if-then, etc.) is a recipe for disaster.
| 38 wrote:
| I would go further to say that syntax should never be used. for
| example with Go:
|
| > cheapFunction(...) || expensiveFunction(...)
|
| is not valid unless both functions return bool
|
| > car = car || "bmw"
|
| is not valid at all, because both types would need to be bool
|
| > funcA(...) && funcB_WhichMightBreakWithoutFuncA(...)
|
| not valid unless functions return bool. I think Go smartly
| realized this syntax is just sugar that causes more problems
| than it solves.
| lolinder wrote:
| This has nothing to do with syntax and short circuiting and
| everything to do with Go's type system. Go, like most
| compiled languages, has no concept of "truthiness".
| JavaScript is not Go and has truthiness.
|
| We can debate the merits of truthiness and using it this way,
| but let's have that debate on the merits, not by invoking
| other languages with completely different design constraints.
|
| Your argument here is similar to what got us "no split
| infinitives" in English (grammarians wanted English to be
| more like Latin).
| 38 wrote:
| just because a footgun exists, doesn't mean you should use
| it
| lolinder wrote:
| Then let's talk--with specifics!--about why it's a
| footgun and we shouldn't use it.
|
| "Because Go doesn't support it" would also be a reason to
| avoid:
|
| * Generics (at least until recently)
|
| * Classes
|
| * Prototypes
|
| * Exceptions
|
| * Async/Await
|
| * Package managers (until recently)
|
| * Algebraic data types
|
| * Effect types
|
| * Type inference
|
| * Type classes
|
| * Etc.
|
| You could argue that one or more of these are footguns,
| but I seriously doubt you'd consider them _all_ to be, so
| let 's talk about what separates the footgun from the
| feature that just didn't fit in Go's design.
| 38 wrote:
| This isn't about Go. This is about a language construct
| (&& and ||) that I would argue is terrible and should
| never be used. Its mainly just sugar, and in my
| experience code heavy on these is harder to read and
| write. Couple that with truthiness and it's even worse.
| trealira wrote:
| Short-circuiting evaluation is also useful for things like
| this: function insertion_sort(a) {
| for (let i = 1; i < a.length; i++) { let key =
| a[i]; let j = i; while (j > 0 &&
| key < a[j-1]) { a[j] = a[j-1];
| j--; } a[j] = key; }
| }
|
| If short circuit evaluation didn't exist, then "key < a[j - 1]"
| would be evaluated even in the case where j = 0, leading to the
| array being indexed out of bounds.
| simonw wrote:
| I don't find this easier to read: !(await
| userService.getUserByEmail(user.email)) ||
| throwError(err.userExists);
|
| I guess if I worked in a codebase that used that pattern
| consistently I'd get used to it pretty quickly, but if I dropped
| into a new codebase that I didn't work on often I'd take a little
| bit longer to figure out what was going on.
| RaftPeople wrote:
| > _I don 't find this easier to read:_
|
| I agree. The previous iteration shown is simpler IMO.
|
| I've really shifted how I code to making things just plain
| simple to look at and understand.
| f1yght wrote:
| That's the way it should be, easy to understand. This set up
| might be short but it's complex to read.
| mega_dean wrote:
| After that step, they say "The resulting code is shorter and
| has no nested logic." The resulting code has the same logic as
| before, it's just not visually represented as being nested.
| I've seen the same argument ("nesting is bad so indentation is
| a code smell") used to say that it's better to use early
| returns and omit the `else` block, eg: if
| (some_condition) { // do stuff here return;
| } // do other stuff here
|
| is "better" than: if (some_condition) {
| // do stuff here } else { // do other stuff
| here }
|
| If you have very-deeply nested code then it usually becomes
| easier to work with after splitting it up into smaller pieces.
| But IMO rewriting code like this to save a single level of
| indentation is bikeshedding.
| jonhohle wrote:
| I usually find the opposite (personally). Get rid of all the
| exceptional cases and error handling up front like you have
| in the first example and then spend the remaining body of the
| function doing the main work of that function.
|
| It's not so much indentation that's an issue, but coupling
| control flow with errors and exceptions.
|
| Swift does a nice job with `guard` statements that basically
| bake this in at the language level - a condition succeeds or
| you must return or throw.
|
| If that control flow is part of business logic, I don't think
| there's any issue with your second example. That's what it's
| there for.
| jonathanlydall wrote:
| I would say (as all good technical people know) it depends.
|
| I have come to appreciate the style of early returns rather
| than else statements as I have found over the years it
| generally makes the code easier for me to follow when I'm
| looking at it possibly years later.
|
| It really depends on the particular condition, but sometimes
| it just reads better to me to not use the else, and this is
| because as a style I tend to try have "fail conditions" cause
| an early return with a success being at the end of the
| method. But again there are regularly exceptions where trying
| to do this "just because" would contort the code, so
| returning an early success result happens often enough.
|
| I have however found that sometimes ReSharper's "avoid
| nesting" suggestion (particularly in examples like yours)
| results in less clear code, but it's almost always at least
| not worse and maybe slightly better for the sake of
| consistency.
|
| EDIT: Having thought about this more, here is why I find
| early returns generally easier to read than else statements.
|
| With an early return the code is generally more linear to
| read as when I get to the end of the if block I can instantly
| see there is nothing else of relevance in the method, I save
| myself having to needlessly scan for the end of the else
| block, or even worse, past more code blocks only to find that
| the rest of the method's code is irrelevant.
|
| Again, not a hard rule, but a consistent style in a code base
| also generally makes it easier to read.
| aidos wrote:
| I call them guard conditions and it's my preferred pattern
| too. Deal with all the exceptional cases at the top and
| then move on to the happy path as a linear flow.
| hollerith wrote:
| And at the start of the happy path, I tend to put "Do it"
| as a comment (a convention I learned from reading Emacs
| Lisp code).
| amonith wrote:
| It could be "dangerous" even sometimes if you're not paying
| attention. In JS/TS "||" operator evaluates the right side when
| the left side is "falsy". "Falsy" doesn't mean only
| null/undefined, but also "", 0, NaN, and... well... false. So
| if you make a method like "isUserActive" or "getAccountBalance"
| and do a throw like that, you'll get an error for valid use
| cases.
| jay_kyburz wrote:
| Whats more, the isUserValid function can't return a more
| detailed error about what was not valid about the User. It
| can only return falsy.
| LorenPechtel wrote:
| Agreed. I do not like that line at all. I might take that
| approach but if I did it would be a separate IsDataValid
| function that checked things, one condition per line. (Might be
| a string of ||s, but never run together like that.) As much as
| possible I want one line to do one thing only.
| tehologist wrote:
| All source code is self documenting, source code is for
| developers to read. Computer languages are a human readable
| version of what the compiler executes and is for the developers,
| not the compilers benefit. As a developer, I read way more
| software than I write and if the source is hard to understand
| then I feel you failed as a developer. Writing is a skill, no
| less important in software than anywhere else. Properly named
| functions/variables and easy to follow flow control is a skill
| that takes years to learn. All developers should keep a thesaurus
| and a dictionary nearby. If you find yourself writing a lot of
| comments trying to explain what you are doing in your code, then
| you probably should refactor.
| virgilp wrote:
| missed the opportunity to create named constants for each of the
| password validation rules.
| cjfd wrote:
| Typescript looks much, much better than what he ends up with. The
| typescript is more or less the same thing but with comment tokens
| removed. How is just removing the comment tokens not an obvious
| improvement in readability?
|
| Honestly, I think all of jsdoc, pydoc, javadoc, doxygen is stuff
| that most code should not use. The only code that should use
| these is code for libraries and for functions that are used by
| hundreds or thousands of other people. And then we also need to
| notice that these docs in comments are not sufficient for
| documentation either. When a function is not used by hundreds or
| thousands of people, just write a conventional comment or perhaps
| not write a comment at all if the function is quite
| straightforward. Documentation that explains the big picture is
| much more important but that is actually somewhat hard to write
| compared to sprinkling jsdoc, pydoc, javadoc or doxygen worthless
| shit all over the place.
| omgJustTest wrote:
| What if the documentation were code? ...
| gnarlouse wrote:
| Having a function throwError makes me squirm.
|
| `isValid() || throwError()` is an abuse of abstraction
| t-writescode wrote:
| A fail fast paradigm is a style of programming that can be used
| very effectively, as long as it's understood that's the style
| of code that is being written.
|
| Much of my code, for example, is fail fast, and then I have
| error handling, supervisors, etc, at a level that can log and
| restart the work.
| 0xbadcafebee wrote:
| "Self-documenting code" is already a thing called Code-as-Docs.
| It's the inverse of Docs-as-Code, where you're "writing
| documentation like you write code". Code-as-Docs is where you
| write Code that is self-documenting. (And this has absolutely
| nothing to do with Literate Programming.)
|
| You do not have to adhere to any specific principles or methods
| or anything specific in order to do Code-as-Docs. Just write your
| code in a way that explains what it is doing, _so that you don 't
| need comments to understand it_.
|
| This often means refactoring your code to make it clearer what it
| does. It may not be what your ideal engineer brain wants the code
| to do, but it will make much more sense to anyone maintaining it.
| Plus very simple things like "variables-that-actually-describe-
| what-they-do" (in a loop over node names, don't make a variable
| called _x_ ; make a variable called _node_name_ )
|
| _edit_ It seems like I 'm the only one who says "Code-as-
| docs"... by searching for "Code-as-documentation" instead of
| "Code-as-docs", I found this:
| https://martinfowler.com/bliki/CodeAsDocumentation.html
|
| I guess "self-documenting code" more hits:
| https://www.google.com/search?q=self-documenting+code
| https://en.wikipedia.org/wiki/Self-documenting_code
| https://wiki.c2.com/?SelfDocumentingCode
| mannyv wrote:
| Code only tells you 'what,' not 'why.' And 'why' is usually what
| matters.
| Vinnl wrote:
| https://github.com/reactjs/react.dev/issues/3896
|
| (I do generally agree with your point.)
| amonith wrote:
| After 10 years as a commercial dev I've noticed I don't really
| care about things like this. Not sure if it ever made a
| difference. The "local code" - as in anything within a function
| or often a single class (1-2k LoC is not really a problem) - is
| trivial to read in most languages. The most difficult thing to
| understand always was the domain or the infrastructure/library
| quirks - stuff that's never properly documented. (Hot take: might
| not be worth to document anyway as it takes longer to write and
| update such docs than to struggle with the code for a little
| bit).
|
| Naming or visual code structure was never a problem in my career
| so far.
| kitd wrote:
| I anticipate the day when Gen AI gives us self-coding
| documentation.
| amonith wrote:
| We created precise artificial languages to avoid programming in
| natural languages which would absolutely suck due to never
| ending ambiguity. I hope that what you're hoping never happens
| :V It would mean the docs would have to turn into "legalese"
| written basically by "code lawyers" - incomprehensible for the
| rest of the team. They would have to write multiple paragraphs
| for simple things like a button on a page just so that AI makes
| precisely what people want - right placement, right color,
| right size, right action, right feedback. And they would have
| to invent a new system for writing references to previously
| defined things just to keep some maintainability. And so many
| more issues...
| johnfn wrote:
| My cut: const passwordRules = [/[a-z]{1,}/,
| /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/]; async function
| createUser(user) { const isUserValid =
| validateUserInput(user); const isPasswordValid =
| user.password.length >= 8 && passwordRules.every((rule) =>
| rule.test(user.password)); if (!isUserValid) {
| throw new Error(ErrorCodes.USER_VALIDATION_FAILED); }
| if (!isPasswordValid) { throw new
| Error(ErrorCodes.INVALID_PASSWORD); }
| const userExists = await userService.getUserByEmail(user.email);
| if (userExists) { throw new
| Error(ErrorCodes.USER_EXISTS); }
| user.password = await hashPassword(user.password);
| return userService.create(user); }
|
| 1. Don't use a bunch of tiny functions. This makes it harder for
| future eng to read the code because they have to keep jumping
| around the file(s) in order to understand control flow. It's much
| better to introduce a variable with a clear name.
|
| 2. Don't use the `a || throw()` structure. That is not idiomatic
| JS.
|
| 2a. Don't introduce `throwError()`. Again, not idiomatic JS.
|
| 3. Use an enum-like object for error codes for clarity.
|
| 4. If we must use passwordRules, at least extract it into a
| global constant. (I don't really like it though; it's a bit too
| clever. What if you want to enforce a password length minimum?
| Yes, you could hack a regex for that, but it would be hard to
| read. Much better would be a list of arrow functions, for
| instance `(password) => password.length > 8`.
|
| 5. Use TypeScript!
| TrianguloY wrote:
| My only nitpick is that the const isPasswordValid = ... should
| be just before its use (between the first two ifs). Other than
| that, I prefer this approach (although I would inline the
| booleans in the ifs to avoid the one-use variables. But that's
| ok).
|
| > Don't use a bunch of tiny functions
|
| Exactly this. I only do that when the function is used in more
| than 10 places and it provides some extra clarity (like
| something as clamp(minVal,val,maxVal){return
| max(minVal,min(val,maxVal))} if your language doesn't already
| have it, of course).
|
| I also apply that to variables though, everything that is only
| used once is inlined unless it really helps (when you create a
| variable, you need to remember it in case it is used
| afterwards, which for me is a hard task)
| xigoi wrote:
| > My only nitpick is that the const isPasswordValid = ...
| should be just before its use (between the first two ifs).
|
| Wouldn't that cause the regexes to be recompiled every time
| you call the function?
| TrianguloY wrote:
| I don't think so, if it does it will now too. In fact with
| my suggestion the regex checks will not run if the user is
| not valid (as it is now it will always run)
| const isUserValid = ... if(!isUserValid) ...
| const isPasswordValid = ... if(!isPasswordValid)
| ...
|
| Etc
| jagged-chisel wrote:
| " Don't use a bunch of tiny functions. This makes it harder for
| future eng to read the code ..."
|
| This is where the naming things bit comes in. You name the
| function correctly, then when the body is read to understand
| that it works as named, you can remove that cognitive
| complexity from your brain and continue on. Once you've built
| trust in the codebase that things do what they claim, you can
| start getting a top-down view of what the code does.
|
| _That_ is the power of proper abstraction.
| johnfn wrote:
| I don't know. I really don't see any clarity improvements
| between, `user.password.length >= 8 &&
| passwordRules.every((rule) => rule.test(user.password))` and
| `validatePassword(password)`. What if you want to add that
| the password must contain one special character? You don't
| actually know, by reading the name "validatePassword", if
| that work has already been done or not. You need to go read
| the function definition and check. And so even for such a
| small function, there is no name that you can choose to truly
| do what you claim and "remove the cognitive complexity from
| your brain".
|
| Once a function gets to a certain threshold of size and
| reuse, I tend to agree with you. But I think that threshold
| is quite large - like at least 40-50 lines, or reused at
| least 3 times.
| jagged-chisel wrote:
| `validatePassword` is definitely clearer. When I'm getting
| familiar with a new codebase, I don't need to know _how_
| each thing happens, I need to know _what_ happens. In fact,
| in your example, you 've already abstracted testing against
| the rules. (AAMOF, the password length requirement should
| be another rule...)
|
| Also with that example, that is about the limit I would
| tolerate in a conditional that decides the next step in
| this app. I just need to know "if the password is valid, go
| to $SCREEN; if not, turn on these messages." I don't care
| about a string of expressions, I care about what it's doing
| and I don't want to parse the code every time to figure out
| "oh, this is just password validation."
|
| This is different from verifying that features are
| implemented - now I need to know the minutia.
| strken wrote:
| It is definitely not clearer to me. I have no idea _what_
| happens in the `validatePassword` function. Does it make
| a synchronous network call that will stop the world for
| half a second? Does it throw an exception, or return an
| error object? I will also have to search the rest of the
| code to see who else calls it, and potentially refactor
| those callers as well.
|
| Any smaller function broken out of a larger function is
| (slightly) confusing. I find that a genuinely large
| function is much more confusing than two functions half
| the size, but a petite little function less than 20 lines
| long is not confusing to begin with.
| LorenPechtel wrote:
| I used to feel that way, but over time I've come to realize
| that for the vast majority of code pulling stuff out so you
| have only one level of nesting and only one pass/fail
| decision makes it easier to understand years later. I'm
| sure the compiler inlines a lot of stuff I pulled out.
|
| The exceptions to this are situations where you need to do
| a bunch of sequential steps and cases where you are looping
| on stuff in higher dimensions.
|
| I have gotten to the point that I consider a comment about
| the code to be a sign of trouble. (Comments about what the
| code is interacting with are a different matter.)
| dietr1ch wrote:
| Abstraction is way better, I don't really want to know how
| the password is validated unless I know I'm facing issues
| with validation (which proper logging tells you about before
| you even dive into the code).
|
| I don't understand why some people prefer being swarmed with
| details. It's not that they want details, but that they just
| hate navigating files (layer 8 + tooling problem) or that
| they "need" to know the details because not knowing them
| haunts them at night somehow.
| OptionOfT wrote:
| My issue with this is that you're using exceptions for control
| flow. A user not being valid is expected (duplicate username).
| A password not matching a regex is also expected.
|
| Then, in general (not seen here as there are no types), I like
| to use a lot of types in my code. The incoming user would be of
| type UnvalidatedUser, whereas the return type of this function
| would be StoredUser or something like that to distinguish the
| incoming user type with the outgoing. I like to attach
| semantics to a type, not to conditions:
| https://existentialtype.wordpress.com/2011/03/15/boolean-bli...
| johnfn wrote:
| That's a great point, I didn't even think of that. I would
| use error types as well, yes.
| rjbwork wrote:
| >1
|
| This is one of my pet peeves. I had an engineer recently wrap
| Dapper up in a bunch of functions. Like, the whole point of
| dapper to me is that it gets out of your way and lets you write
| very simple, declarative SQL database interactions and mapping.
| When you start wrapping it up in a bunch of function calls, it
| becomes opaque.
|
| DRY has been taken as far too much gospel.
| tln wrote:
| I find the comment at the end interesting
|
| // Creates a user and returns the newly created user's id on
| success
|
| Hmm, it returns an id? But the @returns is Promise<any>? The code
| as written will change when userService.create changes... without
| the actual, human readable bit of prose, that potential code
| issue could be easily overlooked.
|
| Of course, here the code could have a newtype for UserId and
| return Promise<UserId>, making the code better and then the prose
| is basically not needed (but please just write a docstring).
|
| FWIW I would document that the `user` parameter is modified. And
| document the potential race condition between checking the
| existence of a user and creating a user, and maybe why it was
| chosen to be done in this order (kinda flimsy in this example).
| Which would probably lead me to designing around these issues.
|
| Trying to only document via self-documenting code seems to always
| omit nuances. /** Create a user and return the
| id, or throw an error with an appropriate code. *
| * user.password may be changed after this function is called.
| */ async function createUser(user: User): Promise<number>
| { if (!validateUserInput(user)) {
| throw new Error(err.userValidationFailed); }
| if (isPasswordValid(user.password)) { // Check
| now if the user exists, so we can throw an error before hashing
| the password. // Note: if a user is created in
| the short time between this check and the actual creation,
| // there could be an unfriendly error const
| userExists = !!(await userService.getUserByEmail(user.email));
| if (userExists) { throw new
| Error(err.userExists); } } else {
| throw new Error(err.invalidPassword); }
| user.password = await hashPassword(user.password);
| return userService.create(user); }
| Savageman wrote:
| This, but move isPasswordValid below if (!isUserValid)
| variadix wrote:
| Types are the best form of documentation because they can be used
| to automatically check for user error, are integral to the code
| itself, and can provide inline documentation. The more I program
| in dynamically typed (or even weakly statically typed) languages
| the more I come to this conclusion.
| einpoklum wrote:
| You're not wrong, but if your code is full of bespoke types
| that are only relevant to a couple of places where they're
| used, you hurt interoperability; you lock yourself in to how
| things are done now; and you may just be shifting the burden of
| making sense to someplace else in the code.
|
| If you are able to formulate types which can be grasped without
| reading and re-reading their code - that's a win; and if you
| are able to express more of your code in terms of more generic
| non-trivial types and data structures, which are language-
| idiomatic - then that's a double win, because you're likely to
| be able to apply library functions directly, write less code,
| and have your code readers thinking "Oh, variadix is just doing
| FOO on the BAR structure, I know what that means".
| dgeiser13 wrote:
| If future "AI" can write code then it should be able to read code
| and describe what it does at various levels of detail.
| jnsie wrote:
| I lived in the C# world for a while and our style guides mandated
| that we use those JSDoc style comments for every function
| definition. I loathed them. They invariable became a more verbose
| and completely redundant version of the function definition.
| Developers even used a tool (GhostDoc, IIRC) to generate these
| comments so that CreateNewUser() became // Create New User.
| Nobody ever read them, few ever updated them, and they reinforced
| my hunch that a small percentage of comments are useful (in which
| case, by all means, use comments!)
| einpoklum wrote:
| I'm a library author and maintainer, in C++ rather than C#. I
| know what you mean about the redundancy, and my solution to
| that - which, of course, requires management approval in your
| case - is the following:
|
| 1. It is legitimate to skip parts of a doxygen/JSDoc comment
| (return value, parameter description) if it is _entirely_
| trivial. But:
|
| 2. You must document the _why_. Well-written function
| signatures can tell you the "what", but they usually not tell
| you the "why".
|
| 3. You must make an effort to squeeze that non-triviality of a
| documented aspect. So, you mention corner-case behavior; you
| relate the use of a parameter to its use elsewhere; and of
| course - you describe "why"'s: Why that parameter is _not_ of
| another type, or why it can't be avoided.
|
| I guarantee you, that if you follow rules (2.) and (3.), you
| will usually not get to apply (1.); and at the same time, your
| doxygen comments will stop being annoying boilerplate that you
| automatically gloss over, because you would be much more likely
| to find useful information there.
___________________________________________________________________
(page generated 2024-10-24 23:00 UTC)