Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Self-Documenting Code

lackofimagination.org

Post reply on HN
Print view View original

81–90 of 131 posts

Re: Self-Documenting Code

#81

Post by hinkley »

amonith wrote: post #30

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…

> Naming or visual code structure was never a problem in my career so far.

Either you're the high verbal skill person on the project and you haven't noticed yet that everyone keeps coming to you to name things, you're in a pocket of devs with high verbal skills so you don't see problems, or you're in an echo chamber where everyone is equally bad and you don't know what 'good' looks like.

There's literally a programmer joke about how hard naming things is. If you don't understand a programmer joke you should probably pull on that thread real hard to figure out why.

Re: Self-Documenting Code

#82

Post by rerdavies »

OptionOfT wrote: post #46
johnfn wrote: post #34

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); } cons…

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 distingu…

My issue with that is that absolutely NOTHING will ever convince me that returning error codes is a better idea than throwing exceptions. And that you seem to be using 'expected' in some weird cargo-culty sense of the word. An invalid user name is an error, not an expected case.

Re: Self-Documenting Code

#83

Post by Aeolun »

> Short-circuit evaluation allows us to simplify conditional statements by using logical operators.

Simple is not the same thing as understandable.

They lost me entirely here.

Re: Self-Documenting Code

#84

Post by hinkley »

mmastrac wrote: post #4

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 perfec…

When I put a hack in place as a workaround for a bug in a dependency, I always link the ticket for the dependency in the PR and the commit comment. If the code looks like someone held a gun to my head or I was on drugs when I wrote it, I'll also put it inline in a comment above the workaround.

Not only to explain why this stupid code is here, but to give the next person permission to delete it if the bug has been fixed upstream and we have pulled the new version down.

Re: Self-Documenting Code

#85

Post by skydhash »

rjbwork wrote: post #68
johnfn wrote: post #34

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); } cons…

>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.

I always prefer having loadBooks than “select * from books” everywhere. I prefer my code to be just the language version of how I would write an explanation if you ask me one specific question. Not for DRY, but for quick scanning and only diving into details when needed

Re: Self-Documenting Code

#86

Post by Chris_Newton »

If I were reviewing the original code, the first thing I’d question is the line

    user.password = await hashPassword(user.password);
1. As a rule, mutations are harder to understand than giving new names to newly defined values.

2. The mutation here apparently modifies an object passed into the function, which is a side effect that callers might not expect after the function returns.

3. The mutation here apparently changes whether user.password holds a safe hashed password or a dangerous plain text password, which are bad values to risk mixing up later.

4. It’s not immediately obvious why hashing a password should be an asynchronous operation, but there’s nothing here to tell the reader why we need to await its result.

At least three of those problems could trivially be avoided by naming the result hashedPassword and, ideally, using TypeScript to ensure that mixing up plain text and hashed passwords generates a type error at build time.

I do agree with many of the other comments here as well. However, I think the above is more serious, because it actually risks the program behaving incorrectly in various ways. Questions like whether to use guard clauses or extract the password check into its own function are more subjective, as long as the code is written clearly and correctly whichever choices are made.

Re: Self-Documenting Code

#87

Post by skydhash »

rerdavies wrote: post #74
einpoklum wrote: post #65

Earlier quoted context omitted.

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…

Could you provide an example of a function or argument type that needs a 'why' explanation. Not getting it.

Not GP, but here is an example of one: https://git.sr.ht/~delthas/senpai/tree/master/item/ui/buffer...

It’s mostly done to mark contentious, complicated, or good enough implementations because after reading the code your first reflex would be: Why is it done that way?

Also personally I mark part of the code with short What? or How? comments for quicker scanning especially if the syntax is terse or it’s not immediately obvious. Kinda like a table of contents.

Re: Self-Documenting Code

#88

Post by nip »

rerdavies wrote: post #82
OptionOfT wrote: post #46

Earlier quoted context omitted.

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 distingu…

My issue with that is that absolutely NOTHING will ever convince me that returning error codes is a better idea than throwing exceptions. And that you seem to be using 'expected' in some weird cargo-culty sense of the word. An invalid user name is an error, not an expected case.

I’ll try:

Returning a “Result” with union discrimination on the “success” is far superior to throwing in Typescript in my experience

   { success: false, reason: “user_already_exists” } | { success: true, data: { user_id: string }
- By design you are forced to “deal” with the sad path: in order to get to the data, you must handle the error state

- Throwing is not type-safe: you cannot know at build time whether you are handling all thrown errors

Re: Self-Documenting Code

#89

Post by yellowapple »

rerdavies wrote: post #82
OptionOfT wrote: post #46

Earlier quoted context omitted.

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 distingu…

My issue with that is that absolutely NOTHING will ever convince me that returning error codes is a better idea than throwing exceptions. And that you seem to be using 'expected' in some weird cargo-culty sense of the word. An invalid user name is an error, not an expected case.

> An invalid user name is an error, not an expected case.

If you ain't expecting users to input bogus data, then you're putting way too much trust in said users.

Put simply: is it a bug in your own code if a user tries to use an invalid username? If yes, then throw an exception. If no, then return an error code. Exceptions represent programmer error; error codes represent user error.

Post reply on HN