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

91–100 of 131 posts

Re: Self-Documenting Code

#91

Post by shiroiushi »

alilleybrinker wrote: post #7
JellyBeanThief wrote: post #6

Earlier quoted context omitted.

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.

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`!

>The kind of pattern used here with the `||` might seem weird to some JavaScript developers, but it's pretty normal in shell scripts

Shell scripts are NOT known for being easy to read. They're full of obscure and sometimes frankly bizarre, arcane syntax that newcomers would have no idea about. Quick, what does "$#" mean? An experienced bash programmer would know, but no one else would. Shell scripts were never meant to be easy to read; they're just an extension of the shell syntax, and of course vary a lot from shell to shell (e.g. bash vs zsh vs ksh etc.).

Re: Self-Documenting Code

#92

Post by fenomas »

jonathanlydall wrote: post #38
mega_dean wrote: post #27

Earlier quoted context omitted.

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 "b…

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

It definitely depends, but personally I find early returns to be a bit of an antipattern IF they're based on business logic. If a function has lots of ifs, you can glance at the nesting to see which ones affect the line you want to edit, and ignore the others. But if the function has lots of returns, you have to check every one before a given line in order to know what constraints are true at that point.

OTOH early returns are great for anything the type checker knows about:

    function foo(msg: 'OK' | 'ERR') {
        // ...
        if (msg === 'ERR') return someValue
        // ...
    }
Doing that is hugely cleaner than branching, and there's no added complexity to the developer since tooling can easily tell you what values `msg` can have at any given point in the function.

Re: Self-Documenting Code

#93

Post by [deleted] »

mmastrac wrote: post #9
alilleybrinker wrote: post #7

Earlier quoted context omitted.

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`!

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

[deleted]

Re: Self-Documenting Code

#94

Post by ilrwbwrkhv »

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…

Something about Javascript and Typescript is really ugly to my eyes. I think it is the large keywords at the beginning of every line. Makes it hard to parse and read. I find C++ style much better to read.

Re: Self-Documenting Code

#95

Post by strken »

hinkley wrote: post #76
strken wrote: post #64

Earlier quoted context omitted.

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 (sl…

The business rules for passwords and usernames are separate. It's okay for them to be separate methods. You also know that the 'valid password' function is going to list the rules for a valid password. If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function?? I don't think you're being…

> If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function??

I don't expect anyone to remember where the code is, regardless of which function it's in. I don't even expect them have been aware of every function to begin with.

How do you expect people will find the `isPasswordValid` function? Because I know from experience that the way I would find it would likely be by looking at the `createUser` function to see what it does. I might do a case-insensitive grep for the word password first, but even then I'd just have to go look at `createUser` anyway, otherwise I wouldn't know whether `isValidPassword` was dead code or referring to a different password.

Re: Self-Documenting Code

#96

Post by eru »

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

If you have nested functions, that's not a problem.

Btw, why do you use regular expressions for some rules, but not for others? Regular expressions are perfectly capable of expressing the length requirement.

Re: Self-Documenting Code

#97

Post by Izkata »

0xbadcafebee wrote: post #24

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

I think "self-documenting code" is older than those other two short terms. I, at least, don't think I've ever heard of them, but I was aware of self-documenting code around 20 years ago in school.

Re: Self-Documenting Code

#98

Post by Izkata »

t-writescode wrote: post #29
gnarlouse wrote: post #19

Having a function throwError makes me squirm. `isValid() || throwError()` is an abuse of abstraction

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.

Not in anything touching user or password validation. It's probably fairly safe the way it's used in this function, but I'd avoid it as a rule of thumb to not accidentally introduce some sort of timing attack.

I'm a little disappointed no one mentioned this even as a side comment, since there's a whole section on doing short-circuit evaluation, even if it's not in a way that would cause this kind of problem.

Re: Self-Documenting Code

#99

Post by Izkata »

I'd like to propose weird alternative to this:

  function throwError(error) {
      throw new Error(error);
  }
  
  async function createUser(user) {
      validateUserInput(user) || throwError(err.userValidationFailed);
      isPasswordValid(user.password) || throwError(err.invalidPassword);
      !(await userService.getUserByEmail(user.email)) || throwError(err.userExists);
What if...

      [
          [() => validateUserInput(user), err.userValidationFailed],
          [() => isPasswordValid(user.password), err.invalidPassword],
          [() => !(await userService.getUserByEmail(user.email)), err.userExists],
      ].forEach(function([is_good, error]) {
          if (!is_good()) {
              throw new Error(error);
          }
      });
Also on the regex:

  const rules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];
No one caught that in all four of these, "{1,}" could be replaced with the much more common "+". A bit odd considering the desire for brevity. I do personally prefer "[0-9]" over "\d", especially considering the other rules, but can go either way on "\W".

I might have also added a fifth regex for length though, instead of doing it differently, if my head was in that mode: /.{8,}/

Re: Self-Documenting Code

#100

Post by swatcoder »

yellowapple wrote: post #89
rerdavies wrote: post #82

Earlier quoted context omitted.

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.

> Exceptions represent programmer error; error codes represent user error.

No, they represent whatever suits the system design and the house style.

They're just tools to pass data and manipulate control flow.

If you become dogmatic and start insisting on what they must absolutely represent, you're only going to find yourself compromising design coherence and futily railing at colleagues when different representations are found to be appropriate.

With exceptions, it often makes sense to use them when failures inevitably just propogate up through a stack of nested calls or may appear anywhere in a sequence of calls. In these cases, return codes can grossly interfere with legibility and can lead to confusion when interim handlers squelch errors that really should have propogates up directly.

These are the kind of choices that are completely reasonable to make within a specific module (like an auth and accounts module), and you can always return to some other default policy at the interface of that module if your project/house-style needs such.

Engineering shouldn't feel so dogmatic as you suggest. It's a strong smell when it does.

Post reply on HN