Colum Ferry

Posted on Apr 14, 2020

Clean JavaScript - 10 Tips

#javascript #cleancode #refactor

We've all been there. We look at our JavaScript from a week, month, year ago and we wonder what kind of coffee we were drinking when we originally wrote it. 🤷‍♂️
A lot of the time, it's down to a mixture of three things: time available to complete work, old best practices or newer patterns and principles for writing our code have come along.

However, there are a few things we can do that will be time-proof and will aid anyone that comes along to our codebase, whether it's future us or a junior developer who's onboarding. I've compiled a list of 10 tips below that I like to employ when writing JavaScript to keep it clean and easy to read.

Complex conditionals? `array.some()` to the rescue

Ok, we've got an if statement and it's pretty verbose. A lot dependent factors on whether we should execute a piece of code. OR, the conditions are dynamically generated from other logic within our app. It's not uncommon to see if statements such as this:

if(condition1
  || condition2
  || condition3 === 'myEquality'
  || ...
  || conditionN.includes('truthy')) {
    // do something
  }

And that can get pretty damn hairy! 🤢
How can we clean that up!? Easy! Arrays!

const myConditions: boolean = [];
myConditions.push(condition1);
myConditions.push(condition2);
myConditions.push(condition3 === 'myEquality');
myConditions.push(conditionN.includes('truthy'));

if (myConditions.some((c) => c)) {
  // do something
}

By creating an array of conditions, we can check if any of them are true, and if so, then the if statement executes. It also means if we need to generate conditions dynamically or via a loop, we can just push to the array of conditions. We can also remove conidtions pretty easily, just comment out the myCondition.push() or remove it completely.

NOTE: This is creating an array and running a loop through the conditions and therefore expect a small, usually unnoticeable, performance impact

Arrays for ORs, but how about ANDs? `array.every()` step up!

Pretty much the same tip as above, execpt instead of just checking that any one of the conditions, array.every() will check that every condition is truthy!

const myConditions: boolean = [];
myConditions.push(condition1);
myConditions.push(condition2);
myConditions.push(condition3 === 'myEquality');
myConditions.push(conditionN.includes('truthy'));

if (myConditions.every((c) => c)) {
  // do something
}

It's as simple as that!

No Magic Strings

Not sure what a magic string is? It boils down expecting an input to be equal to an arbitrary string value that may or may not represent the the implementation and could potentially be used elsewhere, thus making refactors difficult and leading to bug-prone code.
Here's an example of a magic string in action:

function myFunc(input) {
  if (input === 'myString') {
    // do something
  }
}

myFunc('myString'); // works
myFunc('myStrung'); // doesn't work

As you can see from the example above, using the myString magic string can cause bugs to be implemented quite easily. Not only from spelling mistakes from the developer, but also, if you change myFunc by changing the magic string it expects, then everything that calls myFunc will also need to change, or it will break completely:

function myFunc(input) {
  if (input === 'bar') {
    // do something
  }
}

myFunc('myString'); // no longer works
myFunc('myStrung'); // still doesn't work

We can get around this pretty easily but creating a shared object which defines these magic strings with a corresponding key-value setup:

const MY_FUNC_ARGS = {
  DoSomething: 'bar',
};

function myFunc(input) {
  if (input === MY_FUNC_ARGS.DoSomething) {
    // do something
  }
}

myFunc(MY_FUNC_ARGS.DoSomething); // works and is refactor proof!

Not only does defining the magic string in an object provide implementation context to the code, it also helps prevent bugs slipping in via misspellings and refactors! 💪

Array Destructuring Returns

I'm not sure about you, but there has certainly been times where I've wanted to be able to return more than one thing from a function and I either choose to return an array or an object containing the information. For a while I tended to steer away from returning arrays as I hated looking at syntax like this:

const myResult = myFunc();

if (myResult[0] === 'yes' && myResult[1] === 2) {
  // Do something
}

There's no context at all to what the array index of myResult represents and it becomes a little more difficult to understand what is happening here. However, with Array Destructuring we can make this so much more readable 🤓. Check it out:

const [userAnswer, numberOfItems] = myFunc();
if (userAnswer === 'yes' && numberOfItems === 2) {
  // Do something
  // Refactor that magic string to use an Object 🤫
}

Doesn't that make it so much easier to work with!?

Object Destructuring Returns

Ok, Array Destructuring is awesome, and we can get some good context of what's happening because of it, but what if we only care about some of what is returned from the function, and what we do care about is not in the same order as the returned array?

Returning an object might be a better solution here so that we can perform object destructuring on it:

function myFunc() {
  return {
    userAnswer: 'yes',
    numberOfItems: 2,
    someKey: 10,
  };
}

const { numberOfItems, someKey } = myFunc();

if (numberOfItems === 2 || someKey === 10) {
  // Do Something
}

Now, we don't need to care about what order the items exist in the array returned, and we can safely ignore any values preceding the ones we care about 🔥

Many files vs generic files

i.e. The Single Responsibility Principle...
Ok, hear me out. With bundlers, it's painstakingly easy and worthwhile to create new JS files that only do ONE thing, rather than having fewer generic files that do many things.

If you have a file called models.js and it contains objects defining the structure of all the models in your app, consider splitting them out into their own files!
Take this example:

A junior developer is trying to work on the API requests corresponding to adding a TODO item. They have to go into models.js and dig through 1000 lines of code to find the AddTodoRequest object.

A junior developer opens the data-access/todo-requests.js and sees AddTodoRequest at the top of the file.

I know which one I'd prefer! Think about it. Take a look at your files and see if they are doing too much. If so, rip that code into a file more aptly named.

Name your hacks

Ok, so you're trying to do something funky, and there's no suitable method to get it to work. Maybe you have to add a workaround for a specific browser cough IE cough.
You might understand exactly what you have done with a section of code that is specifically for this workaround, but someone coming along after you might have no idea, even you in a months time.

Do yourself, and everyone else, a favour, and name that workaround! It's pretty simple to do, either pull it into a function on it's own or cerate a local variable with a suitable name:

function myIE11FlexWorkaround() {
  /// Workaround code
}

function main() {
  myIE11FlexWorkaround();

  const ie11CssVarsPonyFill = (() => {
    /* some pony fill code */
  })();
}

Now, anyone coming along after you knows exactly what's being attempted! 🚀

Smaller Methods

This goes without saying. I know we all aim to have small methods, but in reality, with time constraints, this can be easier said than done. But, if we flip it on it's head, if we're writing unit tests, I know I'd much rather write a unit test for a small method rather than a large method.

I would much rather see this:

function myLargeComplexMethod() {
  const resultA = doSomePiece();
  const resultB = transformResult(resultA);
  const apiData = mapToApiData(resultB);
  const response = doApiRequest(apiData);
  return response;
}

Than a method that tries to do all these separate units in one go. We can also then write some unit tests for each of these smaller units and write a very simple test fpr myLargeComplexMethod that just ensures that these smaller units are called correctly. We don't need to care if they are working as the unit tests relating to those smaller units will ensure that for us.

`for ... of` vs `forEach`

I think this goes without saying but we've all been burned by callback hell, and .forEach() reminds me too much of callback hell to even want to entertain it. Also, we have a pretty neat way of looping through Iterables of all types now, so why not use it?
Let's see a forEach() in comparison to a for ... of and you can make your own decision.

const myArrayOfObjects = [{ id: 1 }, { id: 2 }, { id: 3 }];
const myMapOfObjects = new Map([
  [1, { id: 1 }],
  [2, { id: 2 }],
  [3, { id: 3 }],
]);

// forEach()

myArrayOfObjects.forEach((obj, index) => {
  // do some code
});

Array.from(myMapOfObjects.values()).forEach((obj, index) => {
  // do some code
});

// For ... of
for (const obj of myArrayOfObjects) {
  // do some code
}

for (const obj of myMapOfObjects.values()) {
  // do some code
}

Personally, I prefer for...of for two reasons:

You can see straight away the intention is to loop through all items in the array
It's consistent for any iterables you have in your codebase, be it an array or a map

forEach does have the benefit of providing the index in the callback, so if that's useful for you, then it might be better to go with that method.

Remove `try-catch` blocks

Finally, a personal gripe of mine. try-catch blocks. I personally feel they are overused, used wrong, they do too much, or catch errors they were never intended to catch, and it's all down to how they are structured and how they look.

I have a much longer description of why I don't like them here, but briefly here's a problematic try-catch:

try {
  const myResult = myThrowableMethod(); // I expect this one to potentially throw
  const response = transformResult(myResult);
  const answer = doRequestThatThrowsButIWasntAware(response); // I didn't realise this could have thrown
} catch (error) {
  console.error(error); // Wait... Which method threw!?
  // do something specifc to handle error coming from myThrowableMethod
  // without expecting the error to be from a different method
}

// Ok, let me refactor so I know for certain that I'm only catching the error I'm expecting
let myResult;

try {
  myResult = myThrowableMethod();
} catch (error) {
  // do something specifc to handle error coming from myThrowableMethod
}

const response = transformResult(myResult);
const answer = doRequestThatThrowsButIWasntAware(response);

Tell me you don't think either of those are problematic... If your error handling logic is in anyway complex, it can just distract the reader from what your method is trying to achieve.

I created a small library to address this: no-try. With it, we can transform the above into:

function handleError(error) {
  console.log(error);
}

const [myResult] = noTry(() => myThrowableMethod(), handleError);
const response = transformResult(myResult);
const answer = doRequestThatThrowsButIWasntAware(response);

Which I personally think is a lot cleaner. But that's a personal thing!

I hope you gain some useful tips from this article that will help you write your JavaScript!

If you have any questions, feel free to ask below or reach out to me on Twitter: @FerryColum.

Top comments (6)

Pragmatic Maciej • Apr 15 '20 • Edited

Replacing conditions by creating additional list structure and pushing items to it 🤔. I would say you are introduction here quite an anti-pattern, and it is visible for me on the first sight.

If you have such many or and and in some point, then the mistake was done earlier, your data model probably is wrongly made, you have too many data relations. The fix is to rethink how you store the data, and putting things into array is not any better.

There is a good use case for arrays instead of raw conditions though. If you have data which represent some variants, lets say something can be in shapeA, shapeB or shapeC then I would consider using array.some in that case. Lets see an example:

// role is a variant with few possible values
if (role === 'Admin' || role === 'Moderator' || role === 'SuperUser') {
  // do smth
}
// with array some we avoid repetition
if (['Admin', 'Moderator', 'SuperUser'].some(allowedRole => allowedRole === role)) {
  // do smth
}

// we can be more readable(subjective) by having isEqual function
const isEqual = a => b => a === b
if (['Admin', 'Moderator', 'SuperUser'].some(isEqual(role))) {
  // do smth
}

But if conditions are about different things, so we check different variables then I would say using array for that has no benefit for standard conditions, and even I would not recommend such approach.

Few thoughts about modeling the data:

Colum Ferry • Apr 15 '20

I think we'll agree to disagree on the usage of arrays. For example, you are using some similar to how I describe where we check an array to see if the condition matches, albeit I state to place the results of conditionals into the array.

As for your isEqual function. That does not look friendly and any junior Dev coming along afterwards will be confused very easily.

The reasoning behind using the array for conditionals stems from situations where we aren't in control of the data model, and yes that can happen, due to time restrictions and due to working on legacy codebases or codebases that have had a lot of employees come and go with little documentation, where product tack new features on without sparing time to handle technical debt.

Pragmatic Maciej • Apr 15 '20

The thing is that it doesn't hep in any way in terms of not controlling the data model, you have trouble if u do that by array or not. My examples of using .some show what I am able to accept in the code, but this examples also do not make any difference in terms of the code quality.

There is no, even single reason to change multi-conditions into multi-array of conditions. I disagree hard here because you have "Clean JavaScript" article title. This is not more clean, but alternative way of doing conditions. If I would see such in the code my first question would be - "Why?". And the only answer I see is - "Because I can"

PS. in terms of isEqual I agree, it is subjective, as point-free not always makes the code better.