DEV Community

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

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

Marcin Kołodziejczak on April 02, 2023

Suspense [when action slows down, uncertainty & tension grows] The 3 Essential Elements of Suspense Explained — How Fincher, Carpent...
Read full post
Collapse
 
dechamp profile image
DeChamp •

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))
Enter fullscreen mode Exit fullscreen mode 
// good
someFunctionThatTakesAnotherFunction(passedFunction)
Enter fullscreen mode Exit fullscreen mode
Collapse
 
ant_f_dev profile image
Anthony Fung •

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

const condition = someCalculation();

if (condition) {
  return true;
} else {
  return false;
}
Enter fullscreen mode Exit fullscreen mode
Collapse
 
tracygjg profile image
Tracy Gilmore • • Edited

Hi Anthony,

It could be worse.

if (!condition) {
  return false;
} else {
  return true;
}
Enter fullscreen mode Exit fullscreen mode

Tracy

Collapse
 
tracygjg profile image
Tracy Gilmore • • 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);
Enter fullscreen mode Exit fullscreen mode

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

function filterTruthful(array = []) {
  return array.filter(Boolean);
}
Enter fullscreen mode Exit fullscreen mode

or simplify the expression as follows.

const filterTruthful = (array = []) => array.filter(Boolean);
Enter fullscreen mode Exit fullscreen mode

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

Collapse
 
kolodziejczakm profile image
Marcin Kołodziejczak • • 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 🙇‍♂️

Collapse
 
michallytek profile image
Michał Lytek • • Edited

Rule 17:
Don't:

const { items: { posts } } = data;
Enter fullscreen mode Exit fullscreen mode

Do:

const { items } = data.posts
Enter fullscreen mode Exit fullscreen mode
Collapse
 
kolodziejczakm profile image
Marcin Kołodziejczak • • 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)

Collapse
 
jcubic profile image
Jakub T. Jankiewicz • • Edited

Your two examples are two different codes:

first:

const posts = data.items.posts;
Enter fullscreen mode Exit fullscreen mode

and the second:

const items = data.posts.items;
Enter fullscreen mode Exit fullscreen mode

but anyway your both examples are ok.

Collapse
 
viacheslavzyrianov profile image
Zyrianov Viacheslav •

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

Collapse
 
kolodziejczakm profile image
Marcin Kołodziejczak • • 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:

  1. 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
  2. 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:

  1. 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-...

  2. 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.

Collapse
 
fruntend profile image
fruntend •

Сongratulations 🥳! Your article hit the top posts for the week - dev.to/fruntend/top-10-posts-for-f...
Keep it up 👍