Hacker News Reader

Comment by progval | Hacker News Reader

I'm not fluent in modern JS, but I think the level of callbacks nesting makes it hard to see what else/catch belong to what if/then, especially: https://github.com/tdjsnelling/dat-keyserver/blob/12fa3e8389...

You could try splitting this big function into smaller functions to reduce the length of the code largest callbacks.

It also makes commits harder to read. eg. for https://github.com/tdjsnelling/dat-keyserver/commit/12fa3e83... a reader can't see easily what changed in the function, as every line's indentation was changed. (And the commit message does not explain what the bug was)

View on HN

Ha, omg this is impossible to read. Using async/await here would go a long way to helping. But honestly this just seems like a work in progress, so code cleanliness and ability for others to read and contribute might not be the focus.

Also, generally people do not put all their logic in the same file , in express apps. Just breaking out the business logic from the express app setup would also help to make it more readable/understandable.

It's also incorrect usage of promises; idiomatically no part of that code should be nested more than two levels deep. But yeah, async/await would be both cleaner to read and easier to write correctly.

(Ultimately a minor issue at this stage of what's otherwise a really neat project!)

Appreciate all the feedback, the code is certainly pretty hacky right now and I intend to clean it up in future. Glad you like the project!

> Ha, omg this is impossible to read

Please edit swipes like this out of your HN comments, particularly in Show HN threads: https://news.ycombinator.com/showhn.html.

I can greatly recommend

  git log -p --color-words
  git show --color-words <commut-ref>

for that problem. There are weaker options to only ignore indentation, iirc.

Diff-so-fancy [1] is even better at visualizing this, in my opinion.

[1] https://github.com/so-fancy/diff-so-fancy

Would agree, my personal preference with this would be functions for each of the if/else cases for some extra readability/documentation.

The linked code block is basically the poster child of why async/await was made.

Even without async/await, it could be better if some unnecessary nesting would be cleaned up

    openpgp.cleartext
      .readArmored(req.body.message)
      .then(message => {
        openpgp.key
          .readArmored(nodes[0].value.key)
          .then(key => {
            ...
          })
      })

could be

    openpgp.cleartext
      .readArmored(req.body.message)
      .then(message =>
        openpgp.key.readArmored(nodes[0].value.key)
      )
      .then(key => {
        ...
      })

Did you try running the code with the change you suggested? The lattermost closure in your snippet (`key => { ... }`) needs to be able to reference `message`, but you've moved it out of scope.

This and similar subthreads in this Show HN entry are a good example of pointless, hostile, obnoxiously-pedantic-without-even-being-technically-correct wankery that makes HN an often terrible place for conversations.

Ugh you're right, it wouldn't work then. For what it's worth I was mostly commenting on the "the poster child of why async/await was made" comment above with the idea that async/await wouldn't necessarily be needed to make the code cleaner. But as you pointed out, in this case it wouldn't work. I'm sorry if the commment came out as wankery.

Reply on news.ycombinator.com