8 Horrifying Practices You Really Must Not Do in JavaScript

JavaScript can be a really flexible but powerful language when developing code but it's not always straight forward to know which practices we should avoid.

In this post we will be going over 8 Practices You Really Must Not Do in JavaScript and should not do before it gets too late.

Some (or all) of the examples in this post might be old news to some of you. But when I see some people using some bad practices like the ones we will go over with here in this post I think this is worth re-visiting so that more of us (including you) can sleep at night.

Without further ado, lets begin!

1. Deep Lookups... without validating

This is one of those practices that could easily be avoided with a tool like TypeScript but surprisingly developers are constantly making this mistake today.

Here is a real world example where someone had recently made in a production app:

if (item['name']['data']['classTag']['name'] == 'Admin') {
  docFormObj['Admin'].map((temp: any) => {
    let itemType: string = item['type']
      .toString()
      .substr(0, item['type'].toString().length - 1)

    let tempType: string = temp['type']
      .toString()
      .substr(0, temp['type'].toString().length - 1)

    if (itemType == tempType) {
      if ((item['type'] & 1) === 1) {
        item.pageName = temp['releaseJump']
      } else {
        item.pageName = temp['privateJump']
      }
    }
  })
}

The problem about this practice is when we do a deep lookup on an object and one of its intermediary values are empty it will produce an error that can crash your app, like this:

const item = {
  name: {
    data: {
      classTag: undefined,
    },
  },
}

if (item['name']['data']['classTag']['name'] == 'Admin') {
  //
}

Result:

TypeError: Cannot read property 'name' of undefined
    at Object.<anonymous> (/Users/abc/def/index.js:9:37)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47

When we look up deep properties on objects it's a good practice to always validate that the data type resulting from a lookup is expected (or at least be sure that each value resulting from a lookup will always either be the expected data type or some falsy value) before proceeding further:

if (
  item['name'] &&
  item['name']['data'] &&
  item['name']['data']['classTag'] &&
  item['name']['data']['classTag']['name'] === 'Admin'
) {
  // Works
}

With optional chaining:

if (item?.['name']?.['data']?.['classTag']?.['name'] === 'Admin') {
  // Works
}

Overdoing it with your folder structure

I know it can be really tempting to start off perfect and strive big when we begin developing our applications especially when we're starting off using a library that scales over time, like redux.

Yes, it's good to prepare your project to become the next biggest thing on the market but if you decide to either take a break or switch to another project you can end up making it harder on yourself (even others as well if they're going to read your code) if you try to go back on it in the future.

Let's take redux for example. If we are building a React application and think about how we want our folder structure to be (keeping in mind that a common convention is to separate components into their own folders) we might end up something similar to this:

The problem is there's a chance that CounterButton, GithubButton and/or Flex might never have to be changed again. If we fast forward 4 months later and our repo contains dozens of folders within folders containing components within more folders it becomes unnecessarily complex. We don't need to be clicking through several folders to look for a component that we are looking for.

If you feel like this is a better approach, and it makes sense, then do it (not because someone on social media told you to):

It should be simple and feel just right for the project you're working with, similarily to how Dan Abramov (creator of redux) says it:

3. Thinking in a box

If we're writing code that people will read (for example when debugging issues with your teammates) do not think inside a box!

I know it is tempting to write functions quickly and get straight to the point. I admit I am guilty of doing this every now and then but I'm also constantly catching myself doing this. If you ever wanted to adopt good coding habits, this one has to make it on your list of top habits to have when coding!

Take a look at this example below:

if (serviceError) {
 resolve(
  new Response({
   code: serviceResponse?.getCode() || -1,
   data: {
    ...responseData,
    error: serviceError,
   },
  }),
 )
} else if (serviceResponse) {

All is good, but when we take a closer look, how do we know what value or data type serviceResponse?.getCode() will return? What if it returns 0? In some conventional practices we're taught that 0 is a code for success (even though languages interpret it as falsy, but that is my point), so we must consider writing our implementations carefully because people won't be able to read our minds and understand clearly what our code is doing. And what if it returns 1, or 5, 9? At this point it's best to not even touch the code and hunt down the developer to explain.

A good solution to write this in a more human readable fashion is to assign the codes as values to constant variables that describe the code and then use them throughout our project.

It's a useful practice that you'll commonly see in many libraries, like twilio-video for example:

module.exports.DEFAULT_ICE_GATHERING_TIMEOUT_MS = 15000
module.exports.DEFAULT_SESSION_TIMEOUT_SEC = 30

module.exports.DEFAULT_NQ_LEVEL_LOCAL = 1
module.exports.DEFAULT_NQ_LEVEL_REMOTE = 0
module.exports.MAX_NQ_LEVEL = 3

module.exports.ICE_ACTIVITY_CHECK_PERIOD_MS = 1000
module.exports.ICE_INACTIVITY_THRESHOLD_MS = 3000

4. Tunnel Visioning While Loops

While loops can be nice when they get the job done quickly it's incredibly devastating when they aren't being careful and missing important handling like when a condition is not met!

Lets take a look at this example below:

let startCount = 0

while (startCount < numComponents) {
  const _component = components[startCount]
  const _node = getByElementId(_component)

  if (!_node) {
    log(
      `Tried to redraw a ${_component.type} component node from the DOM but the DOM node did not exist`,
      { component: _component, node: _node },
    )
  } else {
    const ctx = {} as any
    if (isListConsumer(_component)) {
      const dataObject = findListDataObject(_component)
      dataObject && (ctx.dataObject = dataObject)
    }
    const ndomPage = pickNDOMPageFromOptions(options)
    const redrawed = app.ndom.redraw(_node, _component, ndomPage, {
      context: ctx,
    })

    startCount++
  }
}

The issue here is not incrementing the startCount for the failed condition:

if (!_node) {
  log(
    `Tried to redraw a ${_component.type} component node from the DOM but the DOM node did not exist`,
    { component: _component, node: _node },
  )
}

This means the while loop will never end because startCount will never become increased if numComponents was reached from incrementing startCount at the block below it. I can understand how someone would slip and forget this.

For one, maybe the developer is used to handling code with a simple logging of the failed case (which is actually sufficient for majority of issues we face) and could have accidentally assumed that their program will still run.

Now for my second thought when we write code it usually happens by nature where we end up increasing or updating a variable every time a condition passes because we don't need to do anything further:

let currElem

while (currElem) {
  if (currElem.children.length) {
    currElem = currElem.firstChild
  } else {
    // We don't even need this "else" block
  }
}

So its understandable how the developer in the earlier example had tunnel visioned the passing condition's block.

5. @ts-ignore

If you already use TypeScript and find yourself using this to ignore annoying errors in your code, then believe me this is not the practice to be doing!

There is a more powerful and safer keyword that TypeScript provides to mute linting errors.

By using @ts-expect-error it works the same way @ts-ignore does (or eslint-disable-next-line in ESLint) except it gives you a linting error when it no longer considers it an error without it.

Without @ts-expect-error on a TypeScipt error:

Using @ts-expect-error on a TypeScipt error

Using @ts-expect-error when it is valid:

So why not use the @ts-expect-error approach instead? It protects you and the people around you.

6. Initializing default parameters for invalid values

I mentioned this topic in an earlier article, but this is one of those creepy "gotchas" that can fool a careless developer on a gloomy friday! After all, apps crashing is not a joke--any type of crash can result in money loss at any point in time if not handled correctly.

I was guilty of spending a good amount of time debugging something similar to this:

function init(value = {}) {
  console.log(value.hello)
}

init(null)

Inside init, if value ends up being falsy, it will be initialized with null.

If you're like me, our instincts tell us that value should be initialized to an empty object literal by default if it was a falsy value. But our app will crash when value is falsy because value is null. What?

Default function parameters allow named parameters to become initialized with default values if no value or undefined is passed!

In our case, even though null is falsy, it's still a value!

So the next time you set a default value to null, just make sure to think twice when you do that.

A common practice is just do this:

function init(value = {}) {
  if (value === null) return
  // Continue on with the implementation
  console.log(value.hello)
}

It is short and takes only one line to do. It also provides some more information about the expected data type the function might receive which is a plus because you have one less data type to worry about.

7. instanceof without knowing when it doesn't work

I've been a victim of this before and if you're new to JavaScript theres a good chance you might might not know that instanceof will not work if you use it on objects that were created in a separate execution environment (different browser contexts, global objects, etc).

The mdn documentation lays out a pretty nice example of this behavior in practice:

For instance, [] instanceof window.frames[0].Array will return false, because Array.prototype !== window.frames[0].Array.prototype and arrays inherit from the former

This is something to watch out for because it won't report any errors, it will just silently fail the condition and proceed, making it harder to debug. TypeScript will not catch this either, so you're on your own on this one!

8. async in forEach

I have seen someone do this recently and it's crucial not to do. async does not do what we would expect when used inside forEach calls:

function getAllResponses(...urls) {
  const responses = []

  urls.forEach(async (url) => {
    const resp = await fetch(url)
    const data = await resp.json()
    responses.push(data)
  })

  return responses
}

const responses = getAllResponses()

Running this function will just return an empty array because it doesn't wait for the asynchronous calls to finish. It won't skip the fetching but instead it just won't wait for the calls to finish so there won't be any results returned.

Conclusion

And that concludes the end of this post! I hope you found this to be valuable and look out for more in the future!