/images/node-logo-2.png

TLDR: Why I think init-declarations are a code smell, how to stop it with eslint.

The smell of code

Recently I spotted a merge request with some very poorly structured code. Which lead me to look for some es-lint setting to prevent/mitigate more low quality code. Yeah the cowboy coders have come to town, and started making a mess of the code-base and I'm the sheriff trying to stop them. ๐Ÿ‘ฎ๐Ÿปโ€โ™€๏ธ

The first setting that I've changed is: init-declarations

The docs are here : https://eslint.org/docs/latest/rules/init-declarations

..and now what the cowboy coders did

Yep, these guy's are predicable, they will do anything except write good code. Below is a fictional transcript of what happened.

me: Added es-lint to stop init-declarations, that should stop the cowboys โœ…

cowboy coder : Oh look! My code does not build, โ€ฆhumm, โ€ฆes-lint, ah I see just add "eslint-disable", problem solved. ๐Ÿ˜‡

me: Ok, I've created 5 screen-casts on why init-declarations are a code smell, stop using them and don't try to circumvent the es-lint rules again. ๐Ÿ‘ฎ๐Ÿปโ€โ™€๏ธ

cowboy coder: Oh, everyone uses eslint-disable, no one codes like that. I just looked at what everyone else was doing and copied. ๐Ÿ˜‡

Yeah, this is what happened, and I knew it would. So what gives? Who is the lead-developer on this project. Did the cowboy ( contractor ) offer to clean up the code, to remove the smell, nope.

The problem is down to the following, who is the Boss, who has the power. The only power I have is to prevent their merge requests from getting into the codebase by black balling it. The issue with that? The cowboys then go to the delivery manager, and boom I get over-ruled.

It's even worse when the rest of the team, don't know any better either. But I keep on fighting. ๐Ÿ’ช๐Ÿป

Why add this rule?

The bottom line, to stop cowboy coding from writing garbage code, it won't work, but it will make it much hard for them.

One thing that I have noticed; every function where I find nested if-else statements. I see the following setup line:

  let changedOn;

It's a variable, without an assignment, and it's a precursor to a code smell. But it is itself a code smell. The next line of code after this one, will be an "if", or some ghastly switch statement with multiple branches.

Here is the example above in more detail. Now this code has been wedge in with further code that does more work.

The answer to fixing this is simple, create a function "getChangedOnDate" did you notice? ChangedOn is a date value string. The variable however ChangedOn? well what is it?

    let changedOn;
    if (historyItem?.birthDateGrpStartDtApplDt) {
      changedOn = historyItem.birthDateGrpStartDtApplDt;
    } else if (historyItem?.birthDateStartDt) {
      changedOn = historyItem.birthDateStartDt;
    }
    model.changedOn = changedOn?.replace(/-/g, ' ') || "";
    if (model.changedOn !== "") {
      model.changedOnUnixTime = InputDate.toUnixTime(model.changedOn);
    }
    let changedOn = getChangedOnDateFrom(historyItem);

Now we have a function which tells us where the "changedOn" value comes from, and a function that we can test in isolation.