Part 2 of 2, of "In defense of blocks for local scopes", from https://gist.github.com/getify/712d994419326b53cabe20138161908b

1.md

In defense of blocks for local scopes (Part 2)

Some have leveled the criticism that "part 1" of this post is unnecessarily contriving a problem that doesn't really exist in "good code" -- if you really need to narrow a scope of some declarations in a function, then that function is too complex already and that bigger problem is what you need to fix.

Just to be extra clear: if a chunk of code is already reasonable to make a function, that's not the sort of code I'm talking about in either of these blog posts. If it's being called multiple times, or if it's completely independent and makes sense as its own logical chunk, make it a function. I'm talking about a different sort of code, a set of a few statements related to each other, that declare one or more variables, but which logically still belong inside another function. That's the context here.

OK, let's stop talking about this stuff abstractly.

A Real Example

I'm going to take the React code base as a concrete, real-world, non-contrived example, since it's popular and written by ostensibly pretty smart and good JS developers.

I literally just clicked through their massive code base to the first file I could find that had any substantial code in it, and this was the file, highlighting lines 805-851, the function updateProperties(..).

I'm pasting an abbreviated version of it here so I can illustrate:

export function updateProperties(domElement: Element, updatePayload: Array<any>, tag: string, lastRawProps: Object, nextRawProps: Object, ): void {
  if (
    tag === 'input' &&
    nextRawProps.type === 'radio' &&
    nextRawProps.name != null
  ) {
    ReactDOMInputUpdateChecked(domElement, nextRawProps);
  }

  const wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
  const isCustomComponentTag = isCustomComponent(tag, nextRawProps);
  // Apply the diff.
  updateDOMProperties(
    domElement,
    updatePayload,
    wasCustomComponentTag,
    isCustomComponentTag,
  );

  switch (tag) {
    /* .. */
  }
}

I want to call your attention to the two const declarations in the middle of the function, for wasCustomComponentTag and isCustomComponentTag. Taking a look at the code, those two variables are only used on the very next line, the updateDOMProperties(..) call. They're not used in any of the subsequent (omitted) ~20 lines of switch code.

So... why should those two variables exist in, and be visible to, the function's scope? I argue that this is unnecessary because they aren't needed. Further, I argue it's additional mental overhead a reader has to juggle, because while reading the rest of the function, they have to wonder if either variable will be used again. I suppose the React devs don't care about either, since they didn't bother to narrow the scope?

And perhaps you agree with them?

But, going on the reasoning I firmly hold, that those variables are more visible/exposed than they strictly need to be (see "Principle of Least Privilege" aka "Principle of Least Exposure"), it's at least rational to ask whether that code could/should be a little more readable, and a little more robust/maintainable, if the two declarations were narrowed in scope.

How should we consider doing so? I guess many of you would say "stick those 3 statements in their own function".

OK, but then... two important questions:

1. What should that function be called? I suggest we call it applyTheDiff(..), since that's exactly what the code comment there describes as the purpose of that code:

   function applyTheDiff() {
       const wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
       const isCustomComponentTag = isCustomComponent(tag, nextRawProps);
       // Apply the diff.
       updateDOMProperties(
           domElement,
           updatePayload,
           wasCustomComponentTag,
           isCustomComponentTag,
       );        
   }

   OK, great.

2. But now, should applyTheDiff(..) be located as an inner child function of updateProperties(..)?

   If so, now updateProperties(..) is a bit more complicated to read, in that it has an inner function declaration. And also, that inner function is now using outer-scoped variables (parameters on updateProperties(..)) of domElement and updatePayload. Takes more mental effort for someone to read that code, now that they need to scan up the lexical scope tree to find those identifiers to see where they come from. Not a lot, but a little extra.

   Instead, we could manually pass domElement and updatePayload as arguments into the inner applyTheDiff(..) function, making it clearer where those identifiers come from, but now that's a little more verbose, and a refactoring hazard if those need to be renamed.

   function applyTheDiff(domElement,updatePayload) {
       const wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
       const isCustomComponentTag = isCustomComponent(tag, nextRawProps);
       // Apply the diff.
       updateDOMProperties(
           domElement,
           updatePayload,
           wasCustomComponentTag,
           isCustomComponentTag,
       );        
   }

   If we instead move applyTheDiff(..) up one level as a sibling of updateProperties(..), now we have a different issue: applyTheDiff(..) looks to any reader of the of the code like it can be called directly, just as updateProperties(..) can be called. And the lexical name applyTheDiff (or whatever we pick) could be a future collision hazard with that level of scope, perhaps when someone later wants to define a function of that name in that same scope.

   Moreover, by raising applyTheDiff(..) outside of the scope where it's currently only used (and thus only called once), we have just violated the same "Principle of Least Privilege" that we were trying to avoid in the first place (by placing the two const declarations and updateDOMProperties(..) call inside the applyTheDiff(..) function)!

   We solved one problem and created another in its place. Oops.

Functions used for this single-use purpose will always have this frustrating set of ironic contradictions/questions.

The Block Approach

Let's instead look at that real code example from React with the block-of-scope option in mind:

export function updateProperties(domElement: Element, updatePayload: Array<any>, tag: string, lastRawProps: Object, nextRawProps: Object, ): void {
  if (
    tag === 'input' &&
    nextRawProps.type === 'radio' &&
    nextRawProps.name != null
  ) {
    ReactDOMInputUpdateChecked(domElement, nextRawProps);
  }

  applyTheDiff: {
    const wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
    const isCustomComponentTag = isCustomComponent(tag, nextRawProps);
    updateDOMProperties(
      domElement,
      updatePayload,
      wasCustomComponentTag,
      isCustomComponentTag,
    );
  }

  switch (tag) {
    /* .. */
  }
}

This approach has none of the frustrating questions to juggle that the function approach presents.

Side Note: If you really hate the applyTheDiff block label there, omit it and use a code comment. Makes no different to my larger point.

The big point is, the { .. } block is a better tool for localizing the wasCustomComponentTag and isCustomComponentTag declarations, which had no reason to be exposed to the rest of that updateProperties(..) function.

To repeat/summarize myself from the previous post, this scope-block approach also DOES NOT SUFFER any of these other complexities:

• (Slight) performance overhead of calling a single-use function (in case the JS compiler doesn't inline it)

• Increasing the call-stack in case of debugging an exception

• Altering meaning/behavior of a this, arguments, new.target, super, return, await, yield, break, or continue by wrapping a function boundary around it

The Downside?

The only tangible downside I've seen leveled against this idea is: "it's not common in JS".

It's been common in other languages (C++, Java, etc) for decades. We in JS land should consider adopting it as a best practice from those other languages instead of saying we know better and "functions everything". If we do adopt it, and it catches on wider, the "it's uncommon/weird" argument fades away.

In fact, most "best practices" in some language come from that same adoption process from another language. This has happened for JS many times before, and it could happen again here.

Why such closed-minds on these things? Why not be willing to creatively apply valid JS to address problems differently and maybe even better?

I like it. Sometimes functions need to do a bit and breaking them into smaller functions means lots of variables being passed around. Makes things look like utilities that are actually highly specialised.
In this particular example, there's a solution that doesn't require the 2 const declarations - just call those functions directly as parameters to the function call.
That said, you could easily have a counterexample where those const values need to be used more than once and calling the functions that generated them twice could be wrong in that context.

I think this pattern will start turning up in my code. I'm VERY pedantic about using const but there are certainly cases where it doesn't help and using 3rd party analysers to find possible variable clashes should be a backstop and not a primary mechanism. So this has a nice potential to clean up certain use-cases.

Why such closed-minds on these things? Why not be willing to creatively apply valid JS to address problems differently and maybe even better?

👍 Exactly, give it a try first and then give a feedback. I used them in tests and it completely made sense on some occasions for the test to be more readable. I've never used the labels though, I may give it a try :)

But… Another tiny downside worth considering in performance-sensible parts of code is that creating such a “visibility guard” block scope may use some CPU cycles and a segment of memory internally to create a closure/scope and then clearing it after leaving the scope by the code executor. But in 99% of potential usages it won’t matter.

I would count on the code optimizer to figure it out.

I find that in JS as the langage is so flexible and unrestrictive, any tool we can use to clarify when the useage of something is limited by a clear scope should be considered.

I rely on functions mostly by habit but this seems like a very valid use of scopes, it's like const you could use let everywhere, but it's nice to be able to communicate a bit more about your intent explicitly.

👍

@mieszko4 I like your testing example. I often end up copy/pasting in GET blocks and end up with duplicate variables:

const res = await client.POST(....).expect(200);
res.body.id.should.match('.....');

const res = await client.GET(...).expect(200);
res.body.name.should....

The GET block came from the previous test. So I end up with pres and gres and gres2. Wrapping each in its own scope might be cool. Isolate all those local results. Flipside is that, eg in the above example, if I wanted that id. I'd now need to have a let id outside the block and save it. But that's actually cleaner because it specifically extracts the info from the first half of the test that's required in the second.

Chewing on my own dog food... I just made this little change to an app I'm building, to clarify some local scopes (with labeled blocks):

getify/youperiod.app@7a8ce5a

It's not a huge difference, but I think it helps a little, and I certainly don't think it makes the code less readable in any way.