Why, oh why was this added? | Hacker News Reader

Why, oh why was this added?

zigamiklic.com

102 pointsziga-miklic70 comments

I have this law that I call "Chesterton's Documentation". If the reason for the fence isn't documented, do not be shocked when someone inevitably comes and decides to remove the fence. If you want your fence to stick around: document the rationale. That way, later people know if the reasoning still holds. And if you're at the point where the fence is being removed: might as well bite the bullet. The way I see it, the fallout from the removal (if there is to be any) was sealed at the moment it wasn't documented.

I've come across so many of these in my career. It's not even a rare occurrence (and the author gets this, "And just like that, we are again at step 1 of our debugging journey.") While in the author's case there was a reason for the fence to exist, IME quite often there just isn't. But I've also spent a lot of time working at startups where the mentality was "move fast and clean nothing up".

Sometimes, ^F the thing you're interested in Slack. I've solved a number of code archeology mysteries with that. (But, I doubt it would help here. The author's problem lacks enough context / good search terms. It works better when you have something with a solid name, like a VM.)

Agreed, frequently a single line "n.b." -style comment can save considerable future confusion, toil, and pain by clarifying either the original message intent or facets which might not be immediately obvious to a newcomer.

What does “n.b.” Stand for?

Note well. In Latin, nota bene.

Thank you. I still don’t quite understand what purpose that comment would serve in a codebase

It serves as a notice, like "Look out for a this sharp, hidden edge!"

> If you want your fence to stick around: document the rationale. That way, later people know if the reasoning still holds. And if you're at the point where the fence is being removed: might as well bite the bullet. The way I see it, the fallout from the removal (if there is to be any) was sealed at the moment it wasn't documented

I have a somewhat embarrassing confession: I've done this with code I've written more than once. I've learned to thoroughly comment anything unintuitive even when I know I'm going to be the only one reading it.

The parable of Chesterton's Fence and the parable of cargo cults are in opposition to each other.

One says "if you don't know what it does, don't delete it", the other says "if you don't know what it does, don't keep it".

I disagree. In both cases the observer benefits from reaching beyond their current knowledge. They have the same solution.

Yeah. It is time consuming though. Thankfully our blame tool works well at my job.

Thanks for posting this. Now I have a new interview question to filter out reckless programmers

if you don't know what it does, don't delete it, nor keep it until you know what it does.

Are you saying the only option is to investigate it? The problem with that is then you start deep diving into everything you run into, which takes up a ton of time.

They both cut in the same direction: preserving the status quo. Don’t go around removing stuff you don’t fully understand. And don’t go around adding stuff you don’t understand.

The border between adding and keeping can get blurry. If you're creating a new module based on an existing module, is it appropriate to copy the boilerplate even if you don't understand all of it? What if you're renaming a file, do you have to understand everything in the file?

To the first question, it depends. If it's something you're building for a business, and its reliability is important, then you probably should understand code you're copy-pasting. For a hobby project, it's probably not as important.

The whole point of Chesterton's Fence is that there's wisdom encoded in the way things have been done. The previous guy who worked on the same module was probably somewhat competent, and in any case, the component was fulfilling business needs for all that time, so they were doing something right.

> The way I see it, the fallout from the removal (if there is to be any) was sealed at the moment it wasn't documented.

But the person who removes it have to deal with the aftermath. Midnight callout, fixing corrupted data, etc.

Sure. You don't blindly remove stuff, you do investigate and try to rule out as many possibilities as possible before clicking the "nuke" button.

I.e., you manage the risk.

The alternative — following Chesterton's parable absolutely — means you'll be dragging along, and critically, paying for — both in cash and in complexity of understanding —, an ever-accumulating Katamari of junk that, in all probability, serves no purpose whatsoever. Trying to justify "we want to continue to paying indefinitely for this thing that we don't thing does anything important, but we're not at all sure" — well, at the very least, you're going to look like a clown if you try to pass that by any sort of management that's not asleep at the wheel.

I've removed an awful lot of this stuff in my career. I've gotten remarkably good at deleting a lot of infra with minimal to no impact. Mostly, that comes from first knowing how the actually important infra works, which helps rule out that the odd end over here is going to have any impact at all. Occam's razor helps greatly: asking, "if it did have an impact, how?".

Graceful decoms, such as imaging a VM, or just stopping it for a month prior to out-right deletion, again help build confidence in the non-impact, and reduce the risk if you're wrong: you just turn that VM back on. I.e., having a rollback plan such that if there is some unforeseen contingency, that you can get back to the original state. (Though every now and then, there is an odd service for which you can't do this. Those are annoying.) But even a rollback is good: it provides a valuable clue as to what the unknown thing does, and now you know where to start digging.

It's one of those things where the pragmatic view is the middle ground: wildly deleting things is a poor choice, but so too is blindly keeping things.

Particularly when corps these days want to retain nobody past ~2 years, you end up with a lot of knowledge getting bus factored. If you don't want to get stuck in that quagmire, you've got to be able to clean house.

A personal anecdote:

Our video player has a specific use case where it needs to run at 10x speed. At some point we started seeing decoder underflow issues in Chrome. After banging my head against it for a couple of days, I discovered that setting playbackRate to something just below 10, like 9.9 would make the issue disappear. We didn't have a hard requirement to have it play at exactly 10x, and I didn't want to spend more time on the issue, so I left a descriptive comment next to the fix. Our team now refers to this as "the Warp 10 workaround".

> To make sure you don’t forget, you quickly add a comment explaining why event.stopPropagation() is there. You are hopeful that it will help save time for someone in the future. That future someone might even be you.

This is something I've been training and forcing myself to do. If I spend 10 - 15 minutes thinking about some piece of code or infrastructure to understand why it's here and what it does... add whatever you learned and found out as a comment. Even, or especially if it wasn't fruitful, and your only answer is "the system is being weird here in the following way, and this mess is here to fix it, kind of, in most cases - and since you're here, I guess not in this case".

I admit that I'm a terrible (descriptive) code commenter and that is after 25+ years as a professional programmer, BUT I always try to add a comment when I add some weird code to fix an odd bug or add some special case.

And I can confidently say that pretty much every descriptive comment I've ever written was out-of-date shortly after it was written, but the "hey, this weird because of X and if you think you are going to optimize or fix something you need to be aware of this special case" have saved me numerous times.

Exactly, same here. I'll add a small amount of documentation to the actual code with a reference to a #Gotchas header or equivalent in the readme with verbose detail. Some times design decisions aren't immediately obvious but should be explained for both future colleagues and you.

In the JS ecosystem, where nearly everything published is pre-built, I think it’s time to consider tools for sourcemap’ed git blame or similar. Without leaving my editor I can place my cursor on any line in my own projects, or any line in a clone of a dependency, and get instantaneous information about the last change to that line. The same change in the published package has no way of tracing that information. The usual rigmarole from there is search the package name > no really I want the GitHub link, not NPM > what on earth does the source code structure even look like? > is it even in this repo? > FFS GitHub search is more paternalistic and useless than Google/Bing > drink a whiskey, sleep on it > a full day poring over all of the internals of the dependency, probably a PR to fix something horrifying discovered along the way > shit I should have taken more notes > PR with a version bump in package.json to your fork with the fix, with no comments because this is JSON fuck comments!

Actually wanting events to bubble past a handler is rare enough that I sometimes just through e.stopPropagation() in without thinking too much about it. It's much more common to be bitten by allowing events to continue bubbling IMO

It’s less of a concern for most application devs than it used to be, but still a very widely used technique in libraries/frameworks, because it provides a huge performance boost and vastly simplifies those use cases.

This is one of the most important skills to learn as a developer: when to add a comment explaining the reason for some non-obvious code — and also how to write it so that enough context is given for the future reader, who may not have the same understanding that you have of the rest of the codebase and the business requirements.

Not knowing why some particular piece of code is there, or why it’s coded in that specific way, impedes refactoring and adding/changing features, and in the long run contributes to the ossification of the codebase.

> This is one of the most important skills to learn as a developer: when to add a comment explaining the reason for some non-obvious code — and also how to write it so that enough context is given for the future reader, who may not have the same understanding that you have of the rest of the codebase and the business requirements.

I have the benefit of working on a team with impressively rigorous review habits, and the detriment of being comment-averse when I write code (because I’ve more often than not been bitten by comments that are plainly wrong but only in retrospect).

So this probably doesn’t apply to most, but here’s my heuristic for when to add a code comment: did someone ask a question about it, or something which could be traced back to it? That’s it, the whole comment strategy. I answer questions people actually asked. At which point I generally provide a lot of detail, because if it’s worth asking, it’s worth answering in one place or at least providing a single frame of reference to jump off to other contextually pertinent details.

There are really few times I find stopPropagation() necessary, and try to use it as little as possible. Usually it's something right there in a parent. Even so, the larger point here is that events are very hard to trace through code. This is why I always use enums or static variables to define the names (event types) of events any given class will emit within the class definition. It makes it a lot easier to see what other code references that static name than it does to find every instance of "click" in a huge codebase.

If you only add stopPropagation() when there's a reason, then at least later you know there had to be a reason.

I completely agree. stopPropagation() should be used only as a last resort. I have removed a lot of unneeded use cases with this task, but because of the size of our codebase, many still remain. But at least now they include a comment as to why they exist :)

With enums/static variables are you referring to custom events or also DOM events? For custom-based events we use that approach but not for DOM events. If you do use it for DOM events, I'm curious to know more about it. Thank you!

stopPropagation() should be used only as a last resort

Why? This way you switch from “an event finds its deepest control” mentality to “anything at these coords receives it”, which may have much broader effect under different situations than any reasonable sense may suggest. Is there any programming benefit except pleasing some hacky techniques which go against the fundamental idea?

Anything at given coords should receive the event. Whether it makes sense for some other component to receive the event shouldn't be your component's concern. It introduces extra coupling and is inconsiderate to other potential users of the component, since you don't know how everything composes in the end. Any parent listening can always inspect which particular child triggered the event and decide whether to act based on that, but you can't conjure an event once it's gone or stopped propagating.

I agree with your reasoning mostly, and can remember situations when I wanted to make an event shoot through a finished element but couldn’t, at least not easily (it wasn’t web). But I think that it wasn’t a great idea either to make it the default and burden a programmer with this decision in each handler. The current way of doing it is pretty error-prone and bloats each handler with checks which are otherwise mostly pointless or tedious. This incentivizes non-experts to omit that part and just cargo in stopPropagation() calls anyway.

I should clarify that I still use jQuery in most apps, specifically for its namespaced, off-DOM event model. (I've got my own vanilla version that deals in abortcontrollers but jQuery is handy enough for other things that I end up using it anyway).

So with native events, I don't bother to write enums but I do namespace them and listen for them through the JQ event chain. With custom events I write the enums and listen for them through JQ on the component instance, without having to listen on window or make the component extend EventTarget.

tl;dr I only use custom or namespaced events and never run anything between components/controllers through the DOM's native event chain.

Whenever you have to modify code elsewhere because of a side effect in the current function/file - it should be cross-referenced commented (in both places) because of things like this.

Just because Chesterton’s Fence is a thing doesn’t mean you have to build new ones without signs on them.

I agree completely, unfortunately some APIs (DOM events being a good example) make this unknowable or at least impractical on a timeline one or more commits forward. At least as far as I’m aware, there are no static analysis tools which can find “events which might reference this”. Which isn’t to say your point is hopeless, just that a better strategy is to comment regardless of known cross cutting concerns and get ready to hope.

Nice to say, but the other side in this case would likely be the inside of a third-party library. If it was in-repo code, it could have just been fixed to not require a stopPropagation workaround!

> To make sure you don’t forget, you quickly add a comment explaining why event.stopPropagation() is there.

Not optimal. Better, to make sure you don’t forget, write a test for the case that needs event.stopPropagation() to be there.

Is there a name for "must document" code. For me it's any regex. Every single piece of regex should be documented, no matter how simple. Even if it's only for your own use, you will forget the intent.

I'm fixing up some old shell scripts - any single quotes with variables now get documented - sometimes you want to pass them to another shell or program so it IS intentional.

This type of thing is my response to anyone who says "your code should be clear enough that it doesn't need comments": you can't encode external invariant into source code

I think you're missing the point. People who say that are talking about messy code that needs clarification from comments – those cases likely need to be rewritten to be clearer. This is entirely unrelated to _why_ the code does what it does, which the issue here. That definitely needs comments when it's not obvious.

In the OP's case, the _why_ can be encoded in function names. For example:

    function DatePicker() {
      function handleDateChange(event) {
        preventDialogFoobarFromClosing(event);
        ...
      }

      return (
        <ThirdPartyDatePicker
          onChange={handleDateChange}
        />
      );
    }

    function preventDialogFoobarFromClosing(event) {
      event.preventDefault();
    }

I think those people would say to wrap the stopProgation in a function named.

     doNotTriggerEventInFoobarJS()

> In these cases, I got my answer in the commit history. Someone added an event.stopPropagation() to one event handler and then others copied it to their event handlers. I think they had the same issue I had – they did not know why it was there. They assumed it served a purpose and copied it.

This phenomenon is one of my favorite terms in software: Cargo Cult Programming https://en.wikipedia.org/wiki/Cargo_cult_programming

I tend to do this whenever I am a beginner in a certain framework/language and I am mostly learning from examples without understanding the code deeply.

That phase is over sooner or later, but the code produced in the meantime tends to survive even in production for longer than that. Including the occassional cargo cult shrines.

In one project at a previous company, it took three people more than a week to try to get a third party date picker to work with the application. The funny thing is all the time was spent in writing glue code and debugging that glue code. We had more glue code than we would have code if we had just implemented the date picker internally.

Date Picker is one of those things that seem like a big complicated thing but is actually very very very simple to implement.

Honest advice: implement one yourself and use it in your projects. It doesn't even take 600 lines (even when you include the css).

The "hardest" part is displaying a calendar month (for a given year/month combination), in a grid layout, where columns represent week days. But that is actually not hard at all, because the builtin Date class has all the building blocks necessary to make this work. Even the i18n can be done with the builtin Date class.

Given a (year, month) combination, where month is specified from 1-12, this is how you get the weekday of the first day in the month:

    let firstWeekDay = new Date(year, month - 1).getDay()

This will be a number from 0 to 6, where 0 is sunday.

The other bit of information you need is the last day of the month, which can be obtained thus:

    let lastday = new Date(year, month, 0).getDate()

Now just loop over the range 1...lastday and fill in the grid of the days of the week.

It's that simple.

It's never that simple. For starters, for half the world you need to show Monday as the first day of the week, not Sunday.

Yeah... For some applications that is very important, for others nobody cares. YMMV.

About 2/3 of the people on this planet do not agree with the west about when the year starts, what are the months, and things like that. This may be a huge roadblock for you, or just ignoring it may be perfectly fine.

Hmm, it looks like most countries (incl China) adopt the Gregorian calendar as their official calendar.

India and much of Northern Africa do not adopt the Gregorian calendar. By my estimate, that would be 1/5 of the population as opposed to 2/3.

Hum... It seems things are both better and worse than I expected.

Most of the world officially adopted the Gregorian calendar (one of the regions you missed, the Middle East is on the process of changing, with some countries further than others), but most of those countries have a dual calendar system, so depending on what you do, you may need one or the other.

Yes, for counting population, China is what weights the most, and they almost completely moved already.

It’s very annoying when I pick the left-most column because I assume it’s Monday only to realize that it’s in fact Sunday. I’ve never even heard Americans use the concept, like saying “see you at the start of the week (implied Sunday)”. They do talk about the work week and weekend, though. But maybe the should talk about the week start (Sunday), work week and weekend (only Saturday, maybe Friday evening).

It’s partly my own fault for using the English version of software since I don’t want to use localized GUI navigation guides (they might not exist).

How hard do you think it would be to shift the day of the week? It's just a + and a % operation.

How does this justify installing a 500kb javascript dependency?

Saying this is "not that simple" is just you mentally deciding to lose for no reason.

This is why I would just stick with

    <input type="date">

and let the user agent deal with that complexity

Soooo how many libraries remember to do it? And how many people remember to check the library they chose does it?

For `event.stopPropagation()` the best case scenario is "it makes sense here". In some cases it should be the default. A click handler did something? Stop propagating the event, you wouldn't want to do something else also. These cases don't require documentation. (best-case scenario: your buttons are factored out into some component library that takes care of this for you, so the only `stopPropagation()` calls are interesting ones, ie ones that should have comments).

// Stop propagation of events.

Job done!

// TODO: Remove this when it's no longer needed.

> Nevertheless, one of the things that React 17 changes is how event delegation works.

This is one of the reasons why I only ever touch React or similar frameworks when forced to do so at gunpoint. If a library author is on their 17th major version and is still deciding how core stuff like this works, I have no confidence they're ever going to settle on a long-term solution.

Just give me the standardised web APIs thanks, that's a perfectly suitable framework. At least these converge towards consistency and have been (and will remain) stable for decades. The last thing I want to spend my time on is constantly chasing API breakages.

The web has existed for at least 22 years and we still leave such implementation of "standardised" web APIs entirely to the vendor's discretion. I wouldn't call that converging towards consistency or stable.

So web can’t converge and popular frameworks can’t converge. What’s the answer then, or what sense that makes?

I never claimed to know. My point is that Web APIs aren't the answer, like the person I replied to said.

> If a library author is on their 17th major version and is still deciding how core stuff like this works…

Did you follow the link to understand what the change is, and why they made it? [link]

This isn’t some sweeping rewrite of an entire subsystem for the fourth time (I’m looking at you, Angular); it’s just changing React’s synthetic events to be attached to the react tree’s root node in the DOM instead of directly on the document. They have always been on the document until now. So there is no “still deciding” flip-floppery here, there was a single decision made to better support developers by making one low-impact change. How low-impact?

> We’ve only had to change fewer than twenty components out of 100,000+ in the Facebook product code to work with these changes…[link]

React is very good about semantic versioning. This change, small as it is, is a breaking change for a very small number of users. And that’s why it gets a new major version number.

https://reactjs.org/blog/2020/10/20/react-v17.html#changes-t...

When will people realize how silly it is to conflate the logical flow of their front-ends with the DOM tree? Why are you all letting the logic of your programs be dictated by where you want to place visual elements on the page? Just because you don't feel like a real front-end developer if you don't use React / Angular / Vue / any of the other "component"-based frameworks?

You haven't done any front end development recently I imagine?

The entire purpose of what people do with state management and contexts is to decouple this.

> You haven't done any front end development recently I imagine?

Yes, I do front-end development every day. No, I have not done any React/Angular/Vue development recently. The tunnel-vision idea that "front-end development" must mean using one of the Big Frameworks is exactly the idea I'm pushing back against. I tried them for a while (Angular and Knockout), but wrote them off because they couple the logical organization of your app to the DOM tree and they've never taken any steps to improve that (nor even accept that it's a problem).

If you're talking about Redux, there are good ideas there, but it's really just trading one kind of global state for another.

> The entire purpose of what people do with state management and contexts is to decouple this.

I think I now understand what you're saying. You're saying people spend lots and lots of time trying to work around this exact problem, which is introduced by Angular, React, Vue, et al. Realize that this is entirely an extrinsic problem introduced by "component-based" architecture and that a different approach (HTML as one output of the program) avoids this entirely. Managing state is still hard (as it always is), but there's nothing that needs to be decoupled in the first place. It's similar to how "design patterns" are really workarounds for the limitations of OO programming: Redux is a workaround for the limitations of React.

Could please any budding bloggers add some context to their posts? It takes reading until the middle of the post to understand that, yes, this is a post about React. Don't waste your readers time.

It is very explicitly not a post about React, which is why React was not mentioned until the middle. It is a post about properly documenting your code in order to understand why it is there later.

The example could be any code that interacts with dependencies which get updated from time to time. Instead of “why do we stop propagation here?” the question could have been “why is this float converted to a string here?” If the answer is not clear from within the same function call, it may (may!) need a comment to clarify it.

Code reviews often provide missing context. Unfortunately, git does not store code review comments. We need a FOSS source code manager that integrates code review data.

If someone copied and pasted from somewhere for no reason, not sure a copied and pasted comment is going to help.

Why, oh why do people use precious, content-free headlines?

Reply on news.ycombinator.com