16 don’ts: when JavaScript code review feels like watching a thriller movie
Tags: javascript, programming, codereview, webdev
Published: 2023-04-02 16:33:47

## Suspense
[_when action slows down, uncertainty & tension grows_]
[The 3 Essential Elements of Suspense Explained — How Fincher, Carpenter and Refn Create Suspense - YouTube](https://www.youtube.com/watch?v=FHXh_uy6gT4)

There are multiple ways in which code author can increase reader uncertainty & slow down the review process, for example:

1) Using unnecessary conditions

```TypeScript
const roles = {
  s: 'student',
  t: 'teacher',
};

// bad
const role = currentRole === 's' ? roles.s : roles.t;

// good
// assuming that 'currentRole' can't have different value
// than "s" or "t"
const role = roles[currentRole];
```
2) Wrapping inside conditions also common (static) parts

```TypeScript
// bad
if (firstName === '') {
  getFullName(placeholderName, lastName);
} else {
  getFullName(firstName, lastName);
// Hmm, looks like the only difference is the first argument?
}

// good
const userFirstName = firstName === '' ? placeholderName : firstName;
getFullName(userFirstName, lastName);
```

3) Overusing `optional chaining operator`

```TypeScript
// let's assume that all of 'userData' fields always exist
const userData = {
  id: '123',
  personal: {
    age: 33,
  },
};

// bad
const userAge = userData?.personal?.age; 
// Are those really optional? Should we update TypeScript interfaces?

// good
const userAge = userData.personal.age;
```
4) Overusing `logical OR operator`

```TypeScript
// bad
const filterTruthful = (array) => {
  const a = array || []; // Why not as default function parameter?
  return a.filter(Boolean);
};

const canBeAlsoUndefinedOrNull = 'ABC';
const copyWithFallback = canBeAlsoUndefinedOrNull || ''; 
// Do we need fallback for 0? Is that a possible value?

// good
const filterTruthful = (array = []) => {
  return array.filter(Boolean);
};

const canBeAlsoUndefinedOrNull = 'ABC';
const copyWithFallback = canBeAlsoUndefinedOrNull ?? '';
```
5) Naming data with verbs

```TypeScript
// bad
const verifyUser = {
  id: '123',
  verify: true,
};

validateUser(verifyUser); // Is "validateUser" a higher-order function?

// good
const verifiedUser = {
  id: '123',
  verified: true,
};

validateUser(verifiedUser);
```
6) Choosing too concise names without a reason

```TypeScript
// bad
const p = [
  {
    id: 0,
    c: [[1, 2]],
  },
];

const pc = p.map((i) => i.c.flatMap((c) => c));

// good
const positions = [
  {
    id: 0,
    coordinates: [[1, 2]],
  },
];

const coordinates = positions.map((position) =>
  position.coordinates.flatMap((c) => c)
);
// 'c' is clear enough thanks to pretty obvious context
```
7) Performing type coercions differently & making them easy to overlook

```TypeScript
// bad
const asNumber = +Infinity;
const asBoolean = !!Infinity;
const asString = `${Infinity}`;
const asStringList = [1, 0, ''].map((i) => i.toString());

// good
const asNumber = Number(Infinity); 
// Infinity is a number so this is redundant 
const asBoolean = Boolean(Infinity);
const asString =  String(Infinity);
const asStringList = [1, 0, ''].map(String);
```
8) Using unhelpful (new) conventions

```TypeScript
// bad
const THEME_NAMES = {
  dark: 'dark',
  light: 'light',
};
// Why not camelCase?
// If it: 'is more global const' - do we need const sub-types?
// Do we want changing the name on each scope change?

const Config = {
  themeName: THEME_NAMES.dark,
  simplifiedUI: true,
};
// It's named like a class / constructor.
// Do we really need that capital letter here?

// good
const themeNames = {
  dark: 'dark',
  light: 'light',
};

const config = {
  themeName: themeNames.dark,
  simplifiedUI: true,
};
```
9) Not using comments where they would be helpful (because comments are bad!11)

```TypeScript
// styles.css

/* bad */
.slider {
  transform: translateZ(0);
}

/* good */
.slider {
  transform: translateZ(0); /* Fixes flickering in Safari 14, see: https://github.com/nolimits4web/swiper/issues/3527 */
}
```
However, for some programmers this is not enough and they decide to leave the reviewer with uncertainty to the very end.

## Cliff hanger
[_when plot ends abruptly at a crucial or suspenseful moment, leaving the reader or audience in suspense or uncertainty about the outcome_]

![Screenshot from LOST series which was famous from overusing cliff hangers](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/sx426kr66ut7kccjh17m.jpg)

![Screenshot of LOST series - credits](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/5f3jux8l7vylgp0n24ef.jpg)

10) Wrapping dead-simple operations in multiple, nested functions

```TypeScript
// bad
function validateInput(userInput) {
  return validateText(userInput);
}

function validateText(text) {
  return checkIfString(text);
}

function checkIfString(str) {
  return typeof str === 'string';
}

// good
function validateTextInput(text) {
  return typeof text === 'string';
}
```
11) Using extensive logic / external libs helpers in test files assertions

```TypeScript
// bad
describe("createDate", () => {
  it("returns date in correct format", () => {
    expect(createDate()).toBe(_.trim(moment().format("ll")));
  });
});

// good
// assuming date is properly fixed in test env
// to not fail the day after / in different timezones
describe("createDate", () => {
  it("returns date in format: [three-letter month], [dayNumber], [year]", () => {
    expect(createDate()).toBe("Apr 1, 2023");
  });
});
```
12) Ignoring error handling

```TypeScript
// bad
const getFirstPost = async () => {
  return fetch('https://jsonplaceholder.typicode.com/posts/1');
};

// good
const getFirstPost = async () => {
  let response;
  let parsedResponse;

  try {
    response = await fetch('https://jsonplaceholder.typicode.com/posts/1');
  } catch (e) {
    throw new Error(e); // ConnectionError, e.g. Network connection, CORS
  }

  try {
    parsedResponse = await response.json();
  } catch (e) {
    throw new Error(e); // ParsingError, e.g. SyntaxError
  }

  if (response.ok) {
    return parsedResponse;
  }

  throw new Error(parsedResponse); // ResponseError, e.g. Error from Back-end validation
};
```
Sometimes code author not only makes readers feel uncertain about what is happening in the code, but they also provide reviewers with more-direct false clues.

## Red herring
[_when we are presented with false clue_]
[What is a Red Herring — 5 Techniques to Mislead & Distract an Audience - YouTube](https://www.youtube.com/watch?v=47ntBElzaWk)

>The term was popularized in 1807 by English polemicist William Cobbett, who told a story of having used a strong-smelling smoked fish to divert and distract hounds from chasing a rabbit.
[Wikipedia]

13) Defining `const` that is not used in defined form

```TypeScript
const data = {
  items: {
    posts: [],
  },
};

// bad
const { items } = data;
// ...
return items.posts.map((post) => <div>{post}</div>);

// good
const { posts } = data.items;
// ...
return posts.map((post) => <div>{post}</div>);
```
14) Defining simple condition with current-feature-needs name (premature aliasing)

```TypeScript
const roles = {
  s: 'student',
  t: 'teacher',
};

// bad
const hasAccessToMTS = currentRole === roles.t;

// good
const isTeacher = currentRole === roles.t
```
15) Naming one thing in more than one way
```TypeScript
const roles = {
  s: 'student',
  t: 'teacher',
};

// bad
const userPosition = roles.s;

// good
const userRole = roles.s;
```
16) Leaving misleading comments in the code

```TypeScript
// bad
const performMultipleRequests = async () => {
  const promise1 = await fetch('https://jsonplaceholder.typicode.com/posts/1');
  const promise2 = await fetch('https://jsonplaceholder.typicode.com/posts/2');

  return Promise.all([promise1, promise2]); // performs requests parallely
};

// good
const performMultipleRequests = async () => {
  const promise1 = fetch('https://jsonplaceholder.typicode.com/posts/1');
  const promise2 = fetch('https://jsonplaceholder.typicode.com/posts/2');

  return Promise.all([promise1, promise2]);
};
```

As you can see code review process may resemble watching a thriller movie yet I’m not so sure if that’s always a bad thing… I mean, it’s still better than resembling a horror, comedy or disaster scenes:

{% embed https://www.youtube.com/watch?v=JTZYqODo7o0 %}

which we are trying to avoid by doing the review.

If you like what you’ve read, then I encourage you to check out my previous article:
[Similarities between programming & screenwriting - DEV Community](https://dev.to/kolodziejczakm/similarities-between-programming-screenwriting-k9l)