Hey all! So, I was going through a project codebase handled by a 4 year experienced dev and found a silly code and laughed out of hell seeing it. It is as follows,
if(data.isRequired === true)
Thought of sharing here. Sometimes, as we grow, we forget the basics and all good. By no means, I doubt the dev, but it is very important to review code :)
Top comments (19)
Remember that Javascript is "truthy" in how it handles booleans. Depending on the rest of the code base, performing a hard check here might not actually be a mistake.
Regardless, this could also just be a matter of personal preference, as it has zero impact on the actual execution.
For example, our previous senior dev always reversed the order of their conditionals: if(null === var).
This is a pretty subjective application of logic here, as isRequired might not even be a boolean at all times - would have to see the full context to know.
My thoughts exactly. You might want to check that the value is specifically
true, rather than any other non-falsy value.Agreed! Right to the point Brandin. My point was more to inform all that even the senior dev sometimes do the basic mistake. That's where code review comes handy. Reviews make sure that the quality of code is good.
Coming to isRequired. I always expect high code standards and when it comes to the variable names like isRequired, hasKey, isPresent, etc.. I expect that to be a boolean. It's not good if boolean has non-truthy values which means we don't have the control of the values being stored. As devs, we have to decide which type will be stored in a variable :)
My comment was more to indicate that the line in question is hard to evaluate as an objective "mistake", and I personally would not evaluate it as such.
As for the variable names, there is often a very wide gap between what we as developers expect a value to be, and what it actually is, especially in a loosely typed language like Javascript.
If we ourselves don't know what's being stored, then it's a matter of concern. If used JS in the right way, it will help to do wonders. Else, get ready to fix bugs mate, lol. I had the same mindset during the early stage of my career. Way to go.
I have the same opinion as Brandin as a dev of 15 years. Also your way of communicating is quite patronising. In a language like JS, this kind of conditional makes sense, especially in bigger projects with multiple developers. It adds no weight but is a stricter check that helps to prevent bugs. If this value is changed by someone to a non-truthy value for whatever reason, a code review will most likely not catch it and even your tests might work and you have an extremely hard to track bug. Software projects are never perfect and you should never assume so.
In our company of 150+ devs, the strict checks in loosely typed languages are actually in the coding standards and will get flagged if you don't do them.
Senior developer in 4 years? Doubtful.
The worst code written by a senior developer I've seen was probably something I wrote myself at a point where I should have gone home already.
Senior developers are also people. They can have an off day, can get distracted, or depleted their cognitive capacity. At those moments they can turn into juniors.
Not every day is your day. This is where code reviews come handy :)
Don't see anything so bad in this line of the code to blame that developer and especially "laugh out of hell". It's not even a mistake. Besides, all this seniority is not only about technical skills but soft skills a lot and how you treat and respect people (like without laughing about something like that).
Hey Maxim! Looks like you got my message wrong here. It's more about reviewing the code. I didn't comment on the code but went to the dev and we both had a good time laughing. Agreed! soft skills are a must :) Even I write worst codes sometimes and only code reviews help us achieve good quality.
That's just an overlook. I've seen functions that are hundreds of lines long, where I have to scroll and scroll to the right because some lines don't fit in the screen and they are just chaining methods and rubbish in a so-called "fluent" API.
That's a real pain. Code reviews come handy here. Review comments like why functions with hundreds of lines could affect other devs. We should follow a strict rule of a checklist. Pull requests can be created only if the code standards follow the checklist. This makes sure code quality isn't affected. This has done wonders for our team :)
Yeah that isn't all that terrible honestly. I've seen far far worse.
like nested for loops using k & v ( for key and value ), and then something like kInner & kOuter. Or for foo in bar, that looks like it was literally copied out of a tutorial.
It's not a sign of maturity or seniority to laugh at other peoples code. We have all been there, whatever the circumstance, non-optimal code is written in the real world every day.
I don't even see the problem here, in loosely typed languages, I will always use the === operator especially if other people work on the same codebase. Yes, in an optimal world you would not need this, but that does not reflect reality and in this case actually helps preventing hard to track bugs if you make the condition strict.
There is no problem in code.
So 4year experienced author is not naive!
Mine
HAHA! I felt the same when I re-opened the project which I worked 6 months ago. Code quality increases as time goes.
my code, though it solved the issue permanently bu was written poorly
Sometimes solutions are more important than quality. But, always good to add a comment on why it's done. This might help other devs understand the solution for the fix.