Categories
JavaScript Mistakes

JavaScript Mistakes — Useless Code and Invalid Regex

JavaScript is a very forgiving language. It’s easy to write code that runs but has mistakes in it.

In this article, we’ll look at some useless code that we don’t need to put into our JavaScript code.

No Extra Parentheses

We don’t need parentheses around expressions like arithmetic expressions unless we want to group things that have lower precedence together.

Therefore, we don’t need parentheses on expressions like:

(a * b);

or:

(a * b) + c;

Also, we don’t need parentheses around things like loop variables:

for (let a of (b)){
 //,,,
}

We can remove the parentheses around the b in the loop.

Some operator expressions also don’t need brackets like:

typeof (foo);

We don’t need to wrap parentheses around foo .

Below is another example that has extra parentheses:

(function(){} ? foo() : bar());

We can remove parentheses at the beginning and the end of the line.

If we choose to do an assignment inside a conditional, then we should put parentheses around them to make it clear which variable we’re assigning values to:

while ((foo = bar())) {}

The exception would be a normal for loop, which we write without parentheses:

for (let i = 0; i < 10; i++) {
  //...
}

Like with the assignment within loops, we should also put parentheses around assignment expressions in return statements for clarity:

const foo =(b) => {
  return (b = 1);
}

Remove Extra Semicolons

JavaScript lets us put as many semicolons at the end of the line as we want. It does the same thing as a single semicolon, so the extra semicolons are useless.

We also don’t need semicolons after function declarations.

For instance, we shouldn’t have multiple semicolons in the following line:

let x = 1;;;

Instead, we should write:

let x = 1;

An example of a function declaration is the following code:

function foo() {

};

We don’t need an extra semicolon in the code above. Instead, we should write:

function foo() {

}

Reassigning Function Declarations to a Different Value

Since JavaScript functions are just regular variables, they can also be reassigned to anything.

It’s often an indication of a mistake since we would probably want to call the function rather than assigning it to something else.

For example, we don’t want to write:

function fn() {}
fn = foo;

Instead, we should create a variable explicitly and assign a function to the variable to make it clear that it’s actually a variable that we want to reassign values to:

const fn = `function() {}
fn = foo;`

Photo by Christopher Carson on Unsplash

Assigning Members Imported from Modules to Another Value

Since imported members of JavaScript modules are read-only, we can’t assign them to a new value. Therefore, we should have code like:

import { x, y } from "./foo";
x = 1;

We’ll get the error that x is read-only.

This also applies to the importing of default exports.

For instance:

import x from "./foo";
x = 1;

also give the same error.

The following doesn’t give an error with some build tools:

import * as foo from "./foo";
foo.x = 1;

Function Declarations in Nested Blocks

Before ES6, function declarations in nested blocks are supposed to be in the top level of a program or a body of another function.

However, some JavaScript interpreters let function declarations be added to nested blocks.

For example:

function foo() {
  if (true) {
    function bar() {}
  }
};

isn’t supposed to be valid JavaScript because we have the bar function declaration in the if block of the foo function.

To make our code syntactically correct, we should remove the bar function declaration from the if block. We can also use function expressions within our code blocks if we do want to define a function inside.

Invalid Regular Expressions in RegExp Constructors

We don’t want invalid regular expressions in RegExp constructors. The constructor takes any string and tries to return a regular expression object out of it.

Invalid regular expression definitions include things like:

RegExp(']')
RegExp('-', 'z')

We should make sure we pass in a valid regex string or use regex literals instead.

Conclusion

There’re lots of things that JavaScript interpreters allow, but they’re either useless or causes errors. For example, useless parentheses and semicolons should be removed. Function declarations also shouldn’t be reassigned to new values.

Function declarations also aren’t allowed in blocks, even though JavaScript interpreters may still run it.

Finally, invalid regex strings shouldn’t be passed into the RegExp constructor.

Categories
JavaScript Mistakes

JavaScript Mistakes — Duplicates and Useless Code

JavaScript is a very forgiving language. It’s easy to write code that runs but has mistakes in it.

In this article, we look at some mistakes that people make with conditionals and code blocks.

Duplicate Conditions in if-else-if Chains

We should be careful not to have 2 or more else if blocks with the same condition. This leads to confusion and it’s almost always a mistake. They’ll all evaluate to the same truth value and the later ones will never run.

For instance, if we have:

let x;
const a = false;
const b = true;
if (a) {
  x = 0;
} else if (b) {
  x = 1;
} else if (b) {
  x = 2;
}

Then we’ll see that x is 1 because of the short-circuit evaluation. b is true , so that first else if block that checks for b is run.

Therefore, we should always have different conditions for each else if block. For instance, we should write:

let x;
const a = false;
const b = false;
const c = true;
if (a) {
  x = 0;
} else if (b) {
  x = 1;
} else if (c) {
  x = 2;
}

We should also be careful of duplicates that are caused because of the || and && operator. For instance:

let x;
const a = false;
const b = true;
if (a) {
  x = 0;
} else if (b) {
  x = 1;
} else if (a || b) {
  x = 2;
}

It’s also a duplicate because the last if block’s condition is either the same as a or b .

Duplicate Keys in Object Literals

Duplicate keys should never be in object literals. It’s more typing that doesn’t do any good.

For example, the following object:

const foo = {
  bar: 1,
  bar: 2
};

is the same as { bar: 2 } . The first one is discarded. Other examples include:

const foo = {
  bar: 1,
  'bar': 2
};

or:

const foo = {
  1: 'foo',
  '1': 'bar'
};

Duplicate Case Labels

We shouldn’t have duplicate case labels in our switch statements. For instance, if we have:

const a = 1;
let foo = '';

switch (a) {
  case 1:
    foo = 'a';
    break;
  case 2:
    foo = 'b';
    break;
  case 1:
    foo = 'c';
    break;
  default:
    break;
}

Then foo is 'a' since the switch statement stops evaluating when it encounters the first matching case.

Therefore, the duplicate case label is useless. Instead, we should write:

const a = 1;
let foo = '';

switch (a) {
  case 1:
    foo = 'a';
    break;
  case 2:
    foo = 'b';
    break;
  default:
    break;
}

Empty Block Statements

Empty block statements are useless. Therefore, we shouldn’t include them. For instance:

if (x === 1) {}

We should run something if we have a block.

Empty Character Class in Regex

An empty character class in regex doesn’t match anything, so it probably shouldn’t be in the code. For instance, we write:

const foo = /^foo[]/;

The [] doesn’t do anything so it should be removed.

Reassigning Exceptions in catch Clauses

We shouldn’t reassign the exception object passed in from catch to something else since it obscures the source of the original error. If we assign it to something else, then we wouldn’t be able to determine the origin of the error from this point on.

For instance, if we write:

try {
  // ...
} catch (e) {
  e = 'foo';
}

Then e becomes 'foo' after the assignment. Then we won’t know what e is originally after the reassignment.

Instead, we should assign 'foo' to another variable as follows:

try {
  // ...
} catch (e) {
  let foo = 'foo';
}

Then e has the same value and before and we can also reference 'foo' .

Conclusion

We should never have duplicate conditions in multiple else-if blocks since only the first one in the group of duplicates will do something. This also applies to case labels.

Object key literals also shouldn’t be duplicated since only the last one’s value will remain.

Other useless codes include empty code blocks and empty regex classes.

Finally, we shouldn’t reassign the exception parameter to another value since the original exception value will begone. Instead, we should create a new variable and assign whatever we want to it.

Categories
JavaScript Mistakes

JavaScript Mistakes — Useless Promise Code and Operations

JavaScript is a very forgiving language. It’s easy to write code that runs but has mistakes in it.

In this article, we’ll look at some redundant code that we should remove from our code that we may put in our async functions and other places.

No Unnecessary return await

In JavaScript, async functions always return promises. If we return something there, we return a promise with the resolved value being what we return in the promise.

return await doesn’t do anything except adding extra time before the promise resolves or rejects.

Therefore, we can just return the promise directly.

The only valid use of return await is in a try...catch statement to catch errors from another function that returns a promise.

For instance, the following code is bad:

const foo = async () => {
  return await Promise.resolve(1);
}

as we don’t need the await to return the promise.

We can instead just write:

const foo = async () => {
  return Promise.resolve(1);
}

However, the following use of return await is good:

const foo = async () => {
  try {
    return await Promise.resolve(1);
  } catch (ex) {
    console.log(ex);
  }
}

No Script URLs

Having javascript: URL is a bad practice because it exposes us to the same performance and security issues as eval .

They both let us run arbitrary code from a string, so attackers can do the same.

Also, the performance from running code from a string is slower since the JavaScript interpreter can’t optimize code that’s in a string.

For instance, we should write code like:

location.href = "javascript:alert('foo')";

Instead, just put it inside the JavaScript code as follows:

alert('foo');

No Self Assignment

Self assignments are useless. Therefore, it shouldn’t be in our code.

It’s probably an error that we haven’t caught when we changed our code.

For instance, the following are useless:

a = a;
[a, b] = [a, b];

Instead, we should write something like:

a = b;
[a, b] = [b, a];

No Self Compare

Comparing a variable to itself always returns true . Therefore, it’s pretty useless to have these kinds of expressions in our code.

It’s usually an error from typing or refactoring our code. This may introduce runtime errors in our code.

The only time that it may be appropriate to compare a value against itself is when comparing NaN since NaN doesn’t equal itself when compared with the === operator.

But it’s better to check for NaN using the isNaN or Number.isNaN functions to check for NaN .

isNaN tries to convert its argument to a number before checking for NaN , while Number.isNaN just check for the value without doing anything data type conversion first.

Therefore, we shouldn’t have code like the following:

const a = 1;
`if (a === a) {
    x = 2;
}`

Don’t Use the Comma Operator

The comma operator always returns the last element from the list of items separated by the operator, evaluating them from left to right.

Therefore, it’s not a very useful operator to be using in our code.

We should probably remove any use of it from our code. So something like:

const a = (1, 2, 3);

will always set a to 3.

There aren’t any valid use cases for this operator in a normal program.

Photo by Daiji Umemoto on Unsplash

Only Throw Error Object in throw Expressions

In JavaScript, we can put any value after throw when we try to throw errors.

However, it isn’t good practice to throw anything other than the Error object or a child constructor derived from it.

The Error object automatically keep track of where the Error object was built and originated.

For instance, we should have throw expressions like:

throw "fail";

Instead, we should write:

throw new Error("fail");

Conclusion

return await is mostly useless except for try...catch blocks. Comparing a variable or value to itself is mostly useless, as with assign a variable to itself.

Likewise, the comma operator is also useless as it always returns the last value in the list.

Errors that throw exceptions should always throw an Error object or an instance of its child constructor.

Script URLs in JavaScript is also a bad practice because we’re running code within a string, which prevents any optimizations from being done and it may let attackers run arbitrary with our code.

Categories
JavaScript Mistakes

JavaScript Mistake — Loops, Promises, and More

JavaScript is a very forgiving language. It’s easy to write code that runs but has mistakes in it.

In this article, we’ll look at some JavaScript mistakes, including loops and promises.

Wrong For Loop Direction

A for loop with an ending condition that’ll never be reached is probably buggy code. If we want to make an infinite loop, we should use a while loop as the convention.

For instance, if we have:

for (let i = 0; i < 20; i--) {
}

It’s probably a mistake because we specified the ending condition, but never reach it.

We probably meant:

for (let i = 0; i < 20; i++) {
}

Getters That Don’t Return Anything

If we make a getter but it doesn’t return anything, it’s most likely a mistake. There’s no reason to make a getter that returns undefined .

For instance, the following is probably incorrect:

let person = {
  get name() {}
};

Object.defineProperty(person, "gender", {
  get() {}
});

class Person {
  get name() {}
}

We have useless getters in all of the code above. If we have a getter, then we should return something in it:

let person = {
  get name() {
    return 'Jane';
  }
};

Object.defineProperty(person, "gender", {
  get() {
    return 'female';
  }
});

class Person {
  get name() {
    return 'James';
  }
}

Async Function as a Promise Executor Callback

When we define a promise from scratch, we have to pass in an executor callback function with the resolve and reject functions as parameters.

We don’t want async functions as executors because when errors are thrown, they’ll be lost and won’t cause the newly constructed promise to reject. This makes debugging and handling some errors hard.

If a promise executor function is using await , then it’s usually a sign that creates a new Promise instance is useless or the scope of the new Promise constructor can be used.

What is using await is already a promise and async functions also return a promise, so we don’t need a promise inside a promise.

For example, if we have the following code:

const fs = require('fs');

const foo = new Promise(async (resolve, reject) => {
  fs.readFile('foo.txt', (err, result)=> {
    if (err) {
      reject(err);
    } else {
      resolve(result);
    }
  });
});

const result = new Promise(async (resolve, reject) => {
  resolve(await foo);
});

Then it’s probably a mistake because we’re nesting a promise inside a promise.

What we actually want to do is:

const fs = require('fs');

const foo = new Promise(async (resolve, reject) => {
  fs.readFile('foo.txt', (err, result)=> {
    if (err) {
      reject(err);
    } else {
      resolve(result);
    }
  });
});

const result = Promise.resolve(foo);

Photo by Matthew Henry on Unsplash

No Await Inside Loops

async and await allows for parallelization. Usually, we want to run Promise.all to run unrelated promises in parallel. Running await in a loop will run each promise in sequence. This isn’t necessary for promises that don’t depend on each other.

For instance, we should write:

const bar = (results) => console.log(results);

const foo = async (arr) => {
  const promises = [];
  for (const a of arr) {
    promises.push(Promise.resolve(a));
  }
  const results = await Promise.all(promises);
  bar(results);
}

Instead of:

const bar = (results) => console.log(results);

const foo = async (arr) => {
  const results = [];
  for (const a of arr) {
    results.push(await  Promise.resolve(a));
  }
  bar(results);
}

The first example is a lot faster than the second since we’re running them in parallel instead of sequentially.

If the promises are dependent on each other, then we should use something like the 2nd example.

Don’t Compare Anything Against Negative Zero

Comparing again negative zero will return true for both +0 and -0. We probably actually want to use Object.is(x, -0) to check if something is equal to -0.

For instance, in the following code:

const x = +0;
const y = -0;
console.log(x === -0)
console.log(y === -0)

Both expressions will log true . On the other hand, if we use Object.is as follows:

const x = +0;
const y = -0;
console.log(Object.is(x, -0))
console.log(Object.is(y, -0))

Then the first log is false and the second is true , which is probably what we want.

Conclusion

There’re many ways to write code that works unintentionally with JavaScript. To prevent bugs from occurring, we should use Object.is to compare again +0 and -0, running promises inside promise executor callbacks, adding getters that don’t return anything, or creating infinite loops unintentionally.

If promises can be run in parallel, then we should take advantage of that by using Promise.all .

Categories
JavaScript Mistakes

JavaScript Mistakes — Expressions

JavaScript is a very forgiving language. It’s easy to write code that runs but has mistakes in it.

In this article, we’ll look at some confusing expressions that we shouldn’t be writing in JavaScript code.

Confusing Multiline Expressions

JavaScript has the automatic semicolon insertion(ASI) feature which adds semicolons automatically.

Therefore, we can omit the semicolon and still have a valid JavaScript code. However, this doesn’t mean that they’re easy to read for users.

In JavaScript, a newline character always ends a statement like with a semicolon except when:

  • The statement has an unclose parentheses, array literal, object literal or ends in some other way that’s not a valid way to end a statement
  • The lines is -- or ++
  • It’s a for , while , do , if , or else and there’s no (
  • The next lines start with arithmetic or other binary operators that can only be found between 2 operands

There’re cases where multiline expressions where the new line looks like it’s ending a statement, but it’s not. For instance, the following aren’t are multiline but are actually one expression:

let b = 3
let a = b
(1 || 2).c();

We’ll get a ‘b is not a function’ message since the last 2 lines are interpreted as:

let a = b(1 || 2).c();

Another would be the following:

let addNumber = ()=>{}
let foo = 'bar'
[1, 2, 3].forEach(addNumber);

The code above would get us syntax errors. Therefore, we should put semicolons at the end of each statement so that no developer or JavaScript interpreter would be confused or give errors.

Unreachable Code After return, throw, continue, and break statement

Unreachable code after return , throw , break , and continue are useless because these statements unconditionally exits a block of code.

Therefore, we should never have code that’s never going to be run after those lines.

For instance, the following function has unreachable code:

const fn = () => {
  let x = 1;
  return x;
  x = 2;
}

x = 2 is unreachable since comes after the return statement.

Other pieces of code that we shouldn’t write include:

while(true) {
    break;
    console.log("done");
}

const fn = () => {
  throw new Error("error");
  console.log("done");
}

Therefore, we should write:

const fn = () => {
  let x = 1;
  return x;
}

Control Flow Statements in finally Blocks

In JavaScript, the finally block is always run before the try...catch block finishes when it’s added after a try...catch block. Therefore, whenever we have return , throw , break , continue in finally , the ones in try...catch are overwritten.

For instance, if we have:

let x = (() => {
  try {
    return 'foo';
  } catch (err) {
    return 'bar';
  } finally {
    return 'baz';
  }
})();

Then x would be 'baz' since the finally block’s return statement runs before the ones in the try...catch block.

Therefore we shouldn’t add flow control statements to the finally block since it made the ones in try...catch useless. We should instead write something like:

let x = (() => {
  try {
    return 'foo';
  } catch (err) {
    return 'bar';
  } finally {
    console.log('baz');
  }
})();

so that the return statements in try...catch have a chance to run.

Negating the Left Operand of Relational Operators

Negating the left operand doesn’t always negate the whole operation expression in JavaScript.

For instance:

!prop in object

only negates prop and:

!foo instanceof C

only negates foo .

Therefore, !prop in object is actually true in object or false in object depending on the truthiness of prop .

Likewise, !foo instanceof C is actually, true instanceof C or false instanceof C depending on the truthiness of foo.

Therefore, we should wrap the whole expression in parentheses so it actually negates the return value of the whole expression as follows. Therefore:

!(`prop in object)`

and:

!(`foo instanceof C)`

are what we want.

This way, there’s no confusion about what we’re trying to do with those expressions.

Conclusion

There’re many ways to create confusing expressions with JavaScript. One way is to omit semicolons at the end of the line. Omitting semicolons can create ambiguous expressions easily.

We can also create useless code by writing unreachable expressions. They’re useless so they shouldn’t be written. This include writing code after return , break , throw , and continue . Also, writing those statements in finally also make the ones in try...catch useless.

Finally, negating the left operand of the in and instanceof operators only negate the left operand rather than the whole expression.