DEV Community: Marcin Kołodziejczak

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

Marcin Kołodziejczak — Sun, 02 Apr 2023 16:33:47 +0000

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

Similarities between programming & screenwriting

Marcin Kołodziejczak — Sun, 12 Sep 2021 00:14:53 +0000

This article is most of all focused on the creative process and its interdisciplinary character.
Thus, I’m not going to compare job offers, salaries, prospects and... plenty of other things.
Technical knowledge is not needed though.

Let’s FADE IN!

Collaborative character

💻
You can write a real-world application by yourself.
If you're a go-getter it may be even quite a serious password manager used by over 100,000 people.
That’s the story of Kyle Spearrin the creator of "Bitwarden"¹.

📝
The same stands for screenplays. You can write short films or even feature-length films on your own. In fact, it seems to be quite popular².
Moreover, there are people like Michael Hirst who wrote whole TV series on its own - I'm sure you've heard of the widely beloved “Vikings”.

Nonetheless, most applications and screenplays (especially TV series) are outcomes of close cooperation and that’s why it’s so easy to list the exceptions.

The importance of structure and patterns

💻
There is tons of great content about the application structure.
Flat structure, layered architecture, grouping by modules, grouping by context, hexagonal architecture³…
“Architecture” sounds serious but it mostly boils down to organizing folders and files.
Having known that you placed the files thoughtfully you’re ready to write the code and most likely use some design patterns.
They are battle-tested and well-described ways to solve popular problems. You don’t even have to recognize them, programmers tend to use them without being fully aware.
They are everywhere though.

📝
It’s also the case in screenwriting.
If you’re going to use standard three-act structure you basically know where your turning points should be. You know that you have (at best) 10 minutes to grip the film audience.
In a feature-length film you’re going to have a ~120 pages screenplay in which you’ll have ~40 scenes. You need character objectives for each of them and obstacles to make it dramatic.
However, you don’t have to use that concrete structure and probably won’t when dealing with parallel stories of multiple characters at once⁴.
Full awareness about all of this while writing a screenplay is not essential yet knowing patterns definitely helps in recognizing potential weak points and choosing the right tool for the job.

Every part should be necessary

💻
If you’re a programmer and you worked with properly configured project (actually - a tool used within the project called “linter”) you may have seen similar message more than once:

'doSomethingStrange' is assigned a value but never used. eslint(no-unused-vars)

The linter helps you in identifying unused code so you can easily get rid of it.
We don’t need code which doesn’t serve any function in the current application and only generates additional noise which makes understanding the rest of code harder.
Notice: we don’t need it so much that we’ve made tools to catch it.

📝
If you're a screenwriter I’m sure you’re familiar with the term “Chekhov’s gun”, but for the rest of readers:

If in the first act you have hung a pistol on the wall, then in the following one it should be fired. Otherwise don’t put it there.

Let’s move on.

Reading is more important than writing

💻
It’s often said that reading other people's code will make you a better programmer.
That’s so obvious for experienced ones that it may be even called a truism.
Thanks to that you may find more non-standard approaches to solve specific problems than you know from famous, SOLID books, that I’m sure you’ve read (if you’re a programmer, of course).
But doing self-reviews is not less important.
It’s a good practice to read your code before you convey it to your coworkers. You’ll probably find some room for improvement and do some rewrites.
Then this code is read by your teammates who check it and try to understand.
They may have some suggestions to make it even better or just accept it.
Ultimately, reading makes your application code better.

📝
I don’t like generalising - I know only a few professional screenwriters but they tend to be bookworms. They read a lot not only to get inspired yet to understand other people better and the context in which they live(d).
Writing a convincing story about, let’s say, a “German knight” without knowing anything about the epoch would be hard or even impossible - you need to get familiar with it.
In that case you have to rely on books, because you are probably not going to meet a German knight in upcoming weeks.
Ultimately, screenwriting is just storytelling. Storytelling is based on sharing experiences (they don't have to be ours).

In screenwriting reading also leads to rewrites. Typically screenwriters do several rewrites (presumably more than 3) before the screenplay is ready for production stage (wherein it’s very likely to be partially rewritten), but it highly depends on the way the author works.

Testing

💻
I didn’t mention that before, but during the process of creating new functionality (adding new application code) it’s a good practice to test if that code actually works. To do so, programmers write so-called “unit tests”, which looks like this:

expect(addOneTo(4)).toBe(5);

This is a painfully simple test of one function called “addOneTo” being a tiny part of an application just to show the concept. If the function works correctly this test is going to pass.
Of course that’s not everything.
The whole application is often tested manually by professionals who check all possible usage and seek for bugs.

📝
Screenwriters perform manual tests to check if particular scenes work correctly.
They read dialogues out loud to catch any artificial parts that may be noticed by future audiences.
Sometimes it’s not enough and they test it with a bunch of friends, colleagues or other people who weren’t really involved during the writing phase.

Formatting

💻
Finally! Last thing!
Formatting is crucial for reading and understanding the application code by people but it’s not really important for the computer which executes it.
That’s why it is always a programmer's job to take care of it.
Thankfully there are tools that automate this process (e.g. code is formatted after each CTRL + S by a tool called formatter), therefore programmers don’t have to think about it.

📝
Remember FADE IN from the beginning of the article?
That’s the first two words of a properly formatted screenplay.
Then there is action and dialogue (most likely) and we end everything up with FADE OUT.
There are dozens of rules on how to format a screenplay in a professional way and that’s why screenwriters often use dedicated applications which perform it automatically.
There are also tools based on plain text formatting which is something that you probably would start with if you were a Markdown fanboy (see: Fountain).

And that’s it for now. I hope you liked it! 😃
If you found something unclear don't hesitate to leave a comment - I would love to discuss.

If you like short films then I encourage you to watch the 2-minute thriller of mine.

Cheers! 👋

:qw

FADE OUT

Footnotes:

¹ Yes, Kyle is no longer alone, there is a team which is working on Bitwarden right now.
² Even if there is only one screenwriter in the credits it doesn't have to be the only one person who was writing the screenplay. So-called Script doctors remain uncredited.
³ Great talk about application architecture: https://www.youtube.com/watch?v=oL6JBUk6tj0
⁴ Classic film with aforementioned structure: https://www.imdb.com/title/tt0108122/