Init Declarations
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.