So you just joined a new company, you're excited to learn the newest technology and work on some super cool new projects and then BAM, you have to first learn and navigate the legacy system.
All of a sudden the excitement drains from your body as you're navigating helper file after helper file, unable to make heads or tails of the code base.
In this post I will go over 5 VERY common code smells and how to fix them. If you think something is missing, please check out my previous post, 5 easy wins to refactor even the ugliest code.
1) Conditionals should each get their own line
Generally speaking your code will be a lot easier to read if each statement has its own line. The exception to the rule is to combine the else (or else/if) with the ending of the previous if. However, if you are writing a new if statement, it’s important to put it on a new line. This will prevent any future errors, as it may not be obvious that the two if statements are not logically connected.
2) Properly annotate your optional parameters
Optional parameters can be found in most programming language. Typescript uses the '?', Java uses the 'Optional' type, in PHP you can just assign a default value to a method parameter. In Typescript/Javascript, just like keeping the default clause last in a switch statement, it’s important to follow convention to ease development. When it comes to optional parameters, it’s preferable to use the ‘?’ over the undefined definition.
pssst I tweet about code stuff all the time. If you have questions about how to level up your dev skills give me a follow @mlevkov
3) Watch out for “Dead Stores”
A dead store is when you assign a value to a variable but is then re-assigned without actually using the original value. Calculating or setting a value, without actually using it is at best a waste of resources, and at worst a bug in our code. For the following examples, let's assume we have an array of music tracks and we want to calculate the total runtime of all the songs.
A little added bonus in the following example, is the use of the reduce function to get our value.
4) Don’t Invert Your Booleans
One thing to keep in mind as you are developing, is that you are coding for humans, and not for the compilers. It’s better to keep things as simple and as humanly readable as possible. It’s way too complicated to read when you invert the result of a boolean expression, just use the opposite comparison instead.
5) Use Templates. Don't concatenate!
When concatenating strings you should always stick to string templates instead of the concatenation operator. This will make your life much easier as it allows for multiline strings, reduces any errors if your strings have quotes and is generally a lot easier to read. Here is how it would look like when we try to create a TypeORM connection string without string templates and with.
There you have it, 5 more easy tips that you can apply to almost any codebase.
If you want to level up your coding skills, I'm putting together a playbook that includes:
30+ common code smells & how to fix them
15+ design pattern practices & how to apply them
20+ common JS bugs & how to prevent them
Get early access to the Javascript playbook.
Top comments (5)
Your code in #4 shows off the dangers of refactoring without tests, though. You change
! .. <=into>=where it should just be>. I can't count the number of times I've made that mistake "fixing" other people's code.Well, I could count it, but I'd probably be off by one...
good catch! Will update the gist, many thanks!
One of the hardest parts about refactoring a legacy codebase is not changing the meaning unless you have a test suite available. And let's be real, if it is a true legacy codebase, it either has no tests or there's a file where someone wrote three tests that barely make sense and decided to call it a day.
With #1, by changing the if to else if, you could change the meaning of the code by accident. In this example you are okay, but in other instances, you need to understand why the author chose a chain of if statements instead of else if statements. This could be part of the requirements cleverly hidden in code. I also believe that a switch statement is a much cleaner refactoring here.
For #3, the compiler should optimize out pointless write operations, so I would leave it up to the compiler to do its job and focus on trying to write tests for as much of the existing code as I can.
Sometimes with legacy code, I purposely make one to throw away. I take a copy of the source and refactor mercilessly and recklessly to try to make sense of everything. I will rename variables, comment all over the place, and make a ton of changes to get a better understanding of how things work. At this point, I have made too many changes to assure myself that no program logic has changed, so I delete it or at least set it aside. Even if I didn't make any lasting changes, the understanding gained is priceless.
unfortunately you are right. Legacy code and tests go like oil and water.
regarding #1, that's a great point that I didn't explain well (thanks for the feedback). If you write code that is chained that way, it leads to the next developer to guess why it was written that way.
It's the opposite of self documenting code.
At #2: Java's Optional should not be used as a class field but only as a return type.
See Brain Goetz (Java Language Architect) take on this