5 ms·

There are some polarized opinions in this thread. Someone wrote That might be one of the most readable pieces of code that I've ever read. Apparently it improve

by olavk 8y ago

There are some polarized opinions in this thread. Someone wrote That might be one of the most readable pieces of code that I've ever read. Apparently it improves readability immensely to rename 'forEach' to 'aeach'. To be honest the code is not very readable, but it is very simple and self contained. It would be very easy to dive into to fix a bug because there are no external dependencies or frameworks you have to understand. The list of one-liners at the top seem like a very minimal 'jQuery'-like layer. Perfectly fine except for the l33t identifier names. But there is also some really fragile coupling to the HTML, e.g.: for (var i=0; i < 3; i++) { remEl($(id).nextSibling) } I wouldn't dare to modify the HTML when the JavaScript looks like this! What are the three elements which are being removed? Why exactly three? Probably the author and maintainer of the code knows, but this is the kind of coupling where nobody except the original author would dare to change anything. Perhaps this why HN still uses tables for presentation - nobody dares to change it?

aleksei 8y ago
Perhaps this why HN still uses tables for presentation - nobody dares to change it? There's also simply no reason to change it, it works.
- TeMPOraL 8y ago
  > There's also simply no reason to change it, it works. It works, and it's arguably simpler than the div-soup with extensive styling, which is the "kosher" way of doing this.
  - olavk 8y ago
    Well using divs would at least make you able to group the elements semantically so you could remove a single node rather than having a for loop which removes exactly three nodes. This would be significantly simpler in my opinion. And it would be less fragile since now you can redesign everything inside this node without the JavaScript breaking.
    - Izkata 8y ago
      I expect the layout would also be simpler in another way: Give every "child" div a static indentation, nothing more. They'll nest and the indentation will stack the deeper the comments go. Right now there's already nested tables-within-tables, just to work around how table columns work in order to handle indentation (and each row's indentation is handled separately with a stretched 1x1 image with a width attribute that's calculated server-side).
      - krapp 8y ago
        Or better still, use the HTML elements whose sole purpose is to display nested content: <UL> and <LI>
    - jmiserez 8y ago
      Agreed. But getting divs to do what you want has also gotten considerably easier in the last 10 years. It used to be a lot more work/boilerplate to get a consistent div-based layout across all browsers down to IE 6. As for the redesign, the total amount of code (HTML/CSS/JS) you’d need to touch is still small. Do web designers still exist that only do HTML/CSS but no JS?
      - TeMPOraL 8y ago
        > Do web designers still exist that only do HTML/CSS but no JS? I hope so, given that most websites (as opposed to web apps) in existence should not need JS for core functionality.
      - olavk 8y ago
        > Do web designers still exist that only do HTML/CSS but no JS? That is really beside the point. HTML and JS should not be coupled to the extent that adding or removing some whitespace from the HTML will cause the JS to fail. This kind of things turns a trivial improvement into a nightmare, so you end up not daring to change anything.
  - Klathmon 8y ago
    I'd argue it's far from ideal. Mainly because closing a large tree of comments can often hang the UI for a second or so while this JS runs to find and hide all the children that need to be hidden. But it is simple, and it does get the job done for the most part.
    - dang 8y ago
      It's true that the code is slow on large threads. That's on our list to fix. The last time I profiled it, it was (surprisingly to me) forEach that showed up as the culprit.
      - Klathmon 8y ago
        I figured, and to be honest it's not that big of a deal. I swear it used to be much worse when the collapsing comments first came out, but as I made that comment I realized that I actually had trouble triggering it. Either way, it's a minor annoyance on an otherwise fantastic site!
  - reitoei 8y ago
    Guys, it's 2018. Let's not have this conversation.
- olavk 8y ago
  Sure, don't fix it if it ain't broken. Today there is no reason to use tables for presentation, but that was probably not the case when HN was initially designed many years ago.
- LiterallyDoge 8y ago
  When I last looked into it, tables caused issues for screenreaders and the like.
derekbreden 8y ago
I think the purpose of 'aeach' is not to rename, but to provide a conversion. 'aeach' converts to an array in order to be able to perform a 'forEach' on the non-array NodeLists. The 3 elements being removed are the tr containing the story options, a literal whitespace node, and a tr used as a spacer. I agree it is a coupling, and while accurate to describe as fragile or even really fragile, I personally would not have used those words. I would feel more comfortable daring to make changes in this html and JavaScript than any other front-end code I've worked with in the last 15 years of my career. If for no other reason, the sheer small size of it and lack of external dependencies that you mentioned.
btown 8y ago
You could reach a happy medium if everything was well commented. Introduce a build step to strip comments if you have to. But you would get maintainability while still having full visibility into all the code that is executing.
- dang 8y ago
  It might be interesting to see an example of a comment that you (or anyone) thinks would add to maintainability. What isn't expressed in the code? If there is such information, there are two possibilities: either we could easily modify the code to express it; or we could not. Each would be interesting, but for different reasons.
  - btown 8y ago
    It's probable that HN is so feature-frozen it's not worth doing at this point, but if it were not, there are a few good reasons. The parent's example in hideStory() is a good start: for (var i=0; i < 3; i++) { remEl($(id).nextSibling) } // pop following options row, whitespace text node, .spacer This way, if (for instance) someone were to try to modify the layout to use margins rather than spacers, or if someone were to strip whitespace from the rendered HTML, a quick search (or manual skim of the code when something breaks) for "spacer" or "whitespace" would help someone avoid needing to step through what's being removed in Dev Tools. Minimally invasive, would greatly help someone doing code review, and doesn't even increase line count. Documenting the helper methods at the top would help newcomers to the codebase as well. More generally, going a step further, I've been inspired in my commenting style by Jeremy Ashkenas's literate code for the Backbone library. See, for instance, http://backbonejs.org/docs/backbone.html#section-111 http://backbonejs.org/docs/backbone.html#section-111 . The code itself is as self-explanatory as any other code I've ever seen, but the comments serve to accelerate that browsing process. Backbone, while somewhat dated nowadays, is a great level of abstraction in that the entire library can be grokked in an afternoon - but without comments, I think it would take longer. Reading time does not monotonically increase with number of characters :) And as a third point, if there's ever something confusing enough, or you're using some infrequently-used feature of the CSS/DOM spec, such that the author or reviewer needed to look up how it worked in documentation or a forum somewhere, I encourage my team to simply add that documentation or forum link into the comments. Extremely helpful for maintainability, particularly when onboarding junior team members, allows code reviews of that line to be asynchronous whereas they would otherwise require synchronous explanation, and it's ideally just as quick to keystroke "select-url/copy/close-documentation-tab/tab-into-codebase/start-comment/paste/enter" as it is to "close-documentation-tab/tab-into-codebase." This is Hacker News - we're the antithesis of enforced Javadoc-style comments :) But if you consider someone needing to read code without having the live input of the author, minimal comments for especially terse or creative code can quickly start to make sense, certainly in the territory of the rightmost columns of https://xkcd.com/1205/ https://xkcd.com/1205/
ibejoeb 8y ago
I did a lot of work on HNES for performance, and most of it was assigning meaning to HN's markup. I don't know if it's intentionally opaque or what, but this is just the way it is. The javascript and the html are bound together. It's terse and effective, I suppose, but it was really interesting measuring pixels to judge comment nesting. The JS itself is fine. It's the markup that's a mess.

by olavk 8y ago

aleksei 8y ago

Perhaps this why HN still uses tables for presentation - nobody dares to change it? There's also simply no reason to change it, it works.

TeMPOraL 8y ago
> There's also simply no reason to change it, it works. It works, and it's arguably simpler than the div-soup with extensive styling, which is the "kosher" way of doing this.
- olavk 8y ago
  Well using divs would at least make you able to group the elements semantically so you could remove a single node rather than having a for loop which removes exactly three nodes. This would be significantly simpler in my opinion. And it would be less fragile since now you can redesign everything inside this node without the JavaScript breaking.
  - Izkata 8y ago
    I expect the layout would also be simpler in another way: Give every "child" div a static indentation, nothing more. They'll nest and the indentation will stack the deeper the comments go. Right now there's already nested tables-within-tables, just to work around how table columns work in order to handle indentation (and each row's indentation is handled separately with a stretched 1x1 image with a width attribute that's calculated server-side).
    - krapp 8y ago
      Or better still, use the HTML elements whose sole purpose is to display nested content: <UL> and <LI>
  - jmiserez 8y ago
    Agreed. But getting divs to do what you want has also gotten considerably easier in the last 10 years. It used to be a lot more work/boilerplate to get a consistent div-based layout across all browsers down to IE 6. As for the redesign, the total amount of code (HTML/CSS/JS) you’d need to touch is still small. Do web designers still exist that only do HTML/CSS but no JS?
    - TeMPOraL 8y ago
      > Do web designers still exist that only do HTML/CSS but no JS? I hope so, given that most websites (as opposed to web apps) in existence should not need JS for core functionality.
    - olavk 8y ago
      > Do web designers still exist that only do HTML/CSS but no JS? That is really beside the point. HTML and JS should not be coupled to the extent that adding or removing some whitespace from the HTML will cause the JS to fail. This kind of things turns a trivial improvement into a nightmare, so you end up not daring to change anything.
- Klathmon 8y ago
  I'd argue it's far from ideal. Mainly because closing a large tree of comments can often hang the UI for a second or so while this JS runs to find and hide all the children that need to be hidden. But it is simple, and it does get the job done for the most part.
  - dang 8y ago
    It's true that the code is slow on large threads. That's on our list to fix. The last time I profiled it, it was (surprisingly to me) forEach that showed up as the culprit.
    - Klathmon 8y ago
      I figured, and to be honest it's not that big of a deal. I swear it used to be much worse when the collapsing comments first came out, but as I made that comment I realized that I actually had trouble triggering it. Either way, it's a minor annoyance on an otherwise fantastic site!
- reitoei 8y ago
  Guys, it's 2018. Let's not have this conversation.
olavk 8y ago
Sure, don't fix it if it ain't broken. Today there is no reason to use tables for presentation, but that was probably not the case when HN was initially designed many years ago.
LiterallyDoge 8y ago
When I last looked into it, tables caused issues for screenreaders and the like.

derekbreden 8y ago

I think the purpose of 'aeach' is not to rename, but to provide a conversion. 'aeach' converts to an array in order to be able to perform a 'forEach' on the non-array NodeLists. The 3 elements being removed are the tr containing the story options, a literal whitespace node, and a tr used as a spacer. I agree it is a coupling, and while accurate to describe as fragile or even really fragile, I personally would not have used those words. I would feel more comfortable daring to make changes in this html and JavaScript than any other front-end code I've worked with in the last 15 years of my career. If for no other reason, the sheer small size of it and lack of external dependencies that you mentioned.

btown 8y ago

You could reach a happy medium if everything was well commented. Introduce a build step to strip comments if you have to. But you would get maintainability while still having full visibility into all the code that is executing.

dang 8y ago
It might be interesting to see an example of a comment that you (or anyone) thinks would add to maintainability. What isn't expressed in the code? If there is such information, there are two possibilities: either we could easily modify the code to express it; or we could not. Each would be interesting, but for different reasons.
- btown 8y ago
  It's probable that HN is so feature-frozen it's not worth doing at this point, but if it were not, there are a few good reasons. The parent's example in hideStory() is a good start: for (var i=0; i < 3; i++) { remEl($(id).nextSibling) } // pop following options row, whitespace text node, .spacer This way, if (for instance) someone were to try to modify the layout to use margins rather than spacers, or if someone were to strip whitespace from the rendered HTML, a quick search (or manual skim of the code when something breaks) for "spacer" or "whitespace" would help someone avoid needing to step through what's being removed in Dev Tools. Minimally invasive, would greatly help someone doing code review, and doesn't even increase line count. Documenting the helper methods at the top would help newcomers to the codebase as well. More generally, going a step further, I've been inspired in my commenting style by Jeremy Ashkenas's literate code for the Backbone library. See, for instance, http://backbonejs.org/docs/backbone.html#section-111 http://backbonejs.org/docs/backbone.html#section-111 . The code itself is as self-explanatory as any other code I've ever seen, but the comments serve to accelerate that browsing process. Backbone, while somewhat dated nowadays, is a great level of abstraction in that the entire library can be grokked in an afternoon - but without comments, I think it would take longer. Reading time does not monotonically increase with number of characters :) And as a third point, if there's ever something confusing enough, or you're using some infrequently-used feature of the CSS/DOM spec, such that the author or reviewer needed to look up how it worked in documentation or a forum somewhere, I encourage my team to simply add that documentation or forum link into the comments. Extremely helpful for maintainability, particularly when onboarding junior team members, allows code reviews of that line to be asynchronous whereas they would otherwise require synchronous explanation, and it's ideally just as quick to keystroke "select-url/copy/close-documentation-tab/tab-into-codebase/start-comment/paste/enter" as it is to "close-documentation-tab/tab-into-codebase." This is Hacker News - we're the antithesis of enforced Javadoc-style comments :) But if you consider someone needing to read code without having the live input of the author, minimal comments for especially terse or creative code can quickly start to make sense, certainly in the territory of the rightmost columns of https://xkcd.com/1205/ https://xkcd.com/1205/

ibejoeb 8y ago

I did a lot of work on HNES for performance, and most of it was assigning meaning to HN's markup. I don't know if it's intentionally opaque or what, but this is just the way it is. The javascript and the html are bound together. It's terse and effective, I suppose, but it was really interesting measuring pixels to judge comment nesting. The JS itself is fine. It's the markup that's a mess.