Marcin Kołodziejczak

Posted on Apr 2, 2023

16 don’ts: when JavaScript code review feels like watching a thriller movie

#javascript #programming #codereview #webdev

Suspense

[when action slows down, uncertainty & tension grows]
The 3 Essential Elements of Suspense Explained — How Fincher, Carpenter and Refn Create Suspense - YouTube

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

1) Using unnecessary conditions

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

// 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

// 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

// 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

// 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

// 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

// 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

// 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)

// 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]

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

// 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

// 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

// 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

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

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)

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

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

// bad
const userPosition = roles.s;

// good
const userRole = roles.s;

16) Leaving misleading comments in the code

// 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:

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

Top comments (11)

DeChamp • Apr 3 '23

Great read. Very good points. Loved the examples and you bringing up "Lost" was a nice trip back to memory lane where I watch the show I loved turn into spagetti code lol.

Another classic one i see that I don't believe you mentioned (i checked but maybe missed), is when they add a unnecessary nested function when passing a function where the params will match or have no params.

// bad 
someFunctionThatTakesAnotherFunction((myArg) => passedFunction(myArg))

// good
someFunctionThatTakesAnotherFunction(passedFunction)

Anthony Fung • Apr 3 '23

Great list! Another example I've seen that's similar to (1) is

const condition = someCalculation();

if (condition) {
  return true;
} else {
  return false;
}

Tracy Gilmore • Apr 6 '23 • Edited

Hi Anthony,

It could be worse.

if (!condition) {
  return false;
} else {
  return true;
}

Tracy

Tracy Gilmore • Apr 6 '23 • Edited

Hi Marcin,

Good post, I think I have seen every one of these examples. Here is a little feedback if you do not mind.

Example 2: Why not just have:

getFullName(firstName || placeholderName, lastName);

Example 4: I would suggest either making 'filterTruthful' a conventional function.

function filterTruthful(array = []) {
  return array.filter(Boolean);
}

or simplify the expression as follows.

const filterTruthful = (array = []) => array.filter(Boolean);

Example 5: Be careful with the terminology as validate and verify mean different things - as a test engineer. The way I remember the difference is:
We can verify the user (data) make sense (but this does not ensure the user exists),
we have to validate that the user exists.

Like when we enter a postal code. There is a format the code has to follow (and we can verify that), but that does not mean it is a valid location.

Example 6: Even though the context is reasonably obvious through implication, what is wrong with stating 'coordinate' instead of 'c' and making it explicit.

Example 8: I agree. SNAKE_CASE like this is usually reserved for absolute constants such as DAYS_IN_A_WEEK, HOURS_IN_A_UTC_DAY, etc.

Example 11: Absolutely. The test case should speak for itself and be obvious. Good test cases can provide documentation for the unit subject to test.

Regards, Tracy

Marcin Kołodziejczak • Apr 6 '23 • Edited

Hi Tracy,
Thank you for all your remarks. I answered some of your doubts:

"Why not just have: getFullName(firstName || placeholderName, lastName)"

This: firstName || placeholderName is not an equivalent of presented "bad" part and may suggest a need of placeholder to other untruthful values (e.g undefined or null) which may not be possible.

When it comes to not declaring a const when it is used only once just line below then I generally agree (it depends on e.g. condition length) yet I've done it like this to separate dynamic and static part and make this example as clear as it can be.

"I would suggest either making 'filterTruthful' a conventional function."

TBH I'm against using conventional functions as long as there is no need to use this inside (apply and call don't work on arrow functions).
I don't know why I've used function declarations in this example, but I don't think it's a big deal anyway.

"(...) or simplify the expression as follows."

I don't like that syntax. When I have to add something before the line with return I can't do it just by adding needed line and produce clear GIT diff.
I have to rearrange the function definition and I have to do that manually because code formatter won't help me in that case.

"Be careful with the terminology as validate and verify mean different things (...)"

I agree but I think I didn't suggest that they are the same and this is not the point here. I will think about using different wordings.

"what is wrong with stating 'coordinate' instead of 'c' and making it explicit."

Nothing at all yet I wanted to show the opposite because sometimes programmers tend to treat one letter vars as something that is always wrong (like comments in code are bad!!11 xD).
I don't think that is true and wanted to make a point "by the way".

I hope that my perspective is now more clear 🙇‍♂️

Michał Lytek • Apr 3 '23 • Edited

Rule 17:
Don't:

const { items: { posts } } = data;

Do:

const { items } = data.posts

Marcin Kołodziejczak • Apr 3 '23 • Edited

const { posts } = data.items;

I've already adjusted this snippet because too many people were focused just on nested destructuring (which is not the point and it's more a matter of preference)

Jakub T. Jankiewicz • Apr 9 '23 • Edited

Your two examples are two different codes:

first:

const posts = data.items.posts;

and the second:

const items = data.posts.items;

but anyway your both examples are ok.

Zyrianov Viacheslav • Apr 5 '23

About rule #3, you write let's assume that all of 'userData' fields always exist
Well, what if they don't? Let's assume, it's data from API, that is not 100% stable. So then optional chaining is not so bad at all :D
Sure, even better would be using get() from lodash, but that's whole another story :D

Marcin Kołodziejczak • Apr 5 '23 • Edited

"what if they don't? Let's assume, it's data from API, that is not 100% stable"

From Front-end perspective - before fetched data reaches the view layer (components) it should have gone through 2-step process of preparing data:

Validation - performed e.g. via Zod - if something is wrong with data structure we react accordingly - e.g. via displaying snackbar / inline alert & doing re-fetch
Normalisation - reshaping validated data so we leave only crucial fields with correct names (e.g. only camelCased) and correctly formatted data inside. At this point we can be 100% sure about all data interfaces.

Lack of it OR significant simplification of this 2-step process is often a root cause of importing lodash get or overusing optional chaining operator.

Speaking of lodash, I'm not a big fan of using it, for couple of reasons:

Its functions can be installed / imported in couple of different ways, some of them can have significant impact on app bundle size and lots of devs don't know about that (this could be another point in this article ;p) - twitter.com/filrakowski/status/161...
There are even whole articles that are comparing those ways: blazemeter.com/blog/import-lodash-...
Additionaly lodash has... Two versions :D Yes, that's true. Regular "lodash" which has functions that perform mutations and "lodash/fp" (functional) whose functions aren't performing mutations. The first one may introduce very strange bugs when used in functional workflow e.g. with Redux / Zustand.
The second one has functions named the same way, but... working differently - they have rearranged arguments: github.com/lodash/lodash/wiki/FP-G...
For example: regular lodash: set(object, path, value) vs functional lodash: set(path, value, object)

To sum up - lodash is often generating more problems than it solves and imho - should be used rarely, with caution.