How to Find the Stinky Parts of Your Code [Part XXXI]
source link: https://hackernoon.com/how-to-find-the-stinky-parts-of-your-code-part-xxxi
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
@mcsee
Maximiliano ContieriI’m senior software engineer specialized in declarative d...
Credibility
Code Smells Are a Classic.
It smells because there are likely many instances where it could be edited or improved.
Most of these smells are just hints of something that might be wrong. Therefore, they are not required to be fixed per se… (You should look into it, though.)
Previous Code Smells
You can find all the previous code smells (Part i - XXX) here
Let's continue...
Code Smell 151 - Commented Code
Beginners are afraid to remove code. And many seniors too.
TL;DR: Don't leave commented code. Remove it.
Problems
- Readability
- Dead Code
- Lack of Coverage
- Lack of Source Version Control
Solutions
- Remove commented code
- Implement Source Version Control
Refactorings
Refactoring 005 - Replace Comment with Function Name
Context
When debugging code we tend to comment on code to see what happens.
As a final step, after all our tests pass, we must remove them following clean code practices.
Sample Code
Wrong
function arabicToRoman(num) {
var decimal = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];
var roman = ['M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'];
var result = '';
for(var i = 0; i < decimal.length; i++) {
// print(i)
while(num >= decimal[i]) {
result += roman[i];
num -= decimal[i];
}
}
// if (result > 0 return ' ' += result)
return result;
}
Right
function arabicToRoman(arabicNumber) {
var decimal = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];
var roman = ['M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'];
var romanString = '';
for(var i = 0; i < decimal.length; i++) {
while(arabicNumber >= decimal[i]) {
romanString += roman[i];
num -= decimal[i];
}
}
return romanString;
}
Detection
- [x]Semi-Automatic
Some machine learning analyzers can detect or parse comments and guide as to remove them.
- Comments
Conclusion
We need to remove all commented-out code.
Relations
Code Smell 75 - Comments Inside a Method
Code Smell 05 - Comment Abusers
Credits
Photo by maxim bober on Unsplash
Don’t document the problem, fix it.
Atli Björgvin Oddsson
Software Engineering Great Quotes
Code Smell 152 - Logical Comment
Temporary hacks might be permanent
TL;DR: Don't change code semantics to skip code.
Problems
- Readability
- Non-Intention Revealing
Solutions
- If you need a temporary hack, make it explicit
- Rely on your source control system
Context
Changing code with a temporary hack is a very bad developer practice.
We might forget some temporary solutions and leave them forever.
Sample Code
Wrong
if (cart.items() > 11 && user.isRetail()) {
doStuff();
}
doMore();
// Production code
// the false acts to temporary skip the if condition
if (false && cart.items() > 11 && user.isRetail()) {
doStuff();
}
doMore();
if (true || cart.items() > 11 && user.isRetail()) {
// Same hack to force the condition
Right
if (cart.items() > 11 && user.isRetail()) {
doStuff();
}
doMore();
// Production code
// Either if we need to force or skip the condition
// we can do it with a covering test forcing
// real world scenario and not the code
testLargeCartItems() {}
testUserIsRetail() {}
Detection
- [x]Semi-Automatic
Some linters might warn us of strange behavior.
- Comments
Conclusion
Separation of concerns is extremely important in our profession.
Business logic and hacks should always be apart.
Relations
Code Smell 151 - Commented Code
Credits
Photo by Belinda Fewings on Unsplash
Thanks, @Ramiro Rela for this tip
You might not think that programmers are artists, but programming is an extremely creative profession. It's logic-based creativity.
John Romero
Software Engineering Great Quotes
Code Smell 153 - Too Long Names
Names should be long and descriptive. How Long?
TL;DR: Names should be long enough. No longer.
Problems
- Readability
- Cognitive Load
Solutions
- Use names related to the MAPPER
Context
We used very short names during the 50s and 60s due to space and time constraints.
This is no longer the case in modern languages.
Sometimes we get too excited.
Naming is an art and we should be cautious with it.
Sample Code
Wrong
PlanetarySystem.PlanetarySystemCentralStarCatalogEntry
// Redundant
Right
PlanetarySystem.CentralStarCatalogEntry
Detection
- [x]Semi-Automatic
Our linters can warn us with too long names.
- Bloaters
- Naming
Conclusion
There are no hard rules on name length.
Just Heuristics.
Relations
More Info
Credits
Photo by Emre Karataş on Unsplash
Many people tend to look at programming styles and languages like religions: if you belong to one, you cannot belong to others. But this analogy is another fallacy.
Niklaus Wirth
Software Engineering Great Quotes
Code Smell 154 - Too Many Variables
You debug code and see too many variables declared and active
TL;DR: Variables should be as local as possible
Problems
- Readability
- Code Reuse
Solutions
- Extract Method
- Remove unused variables
Refactorings
Refactoring 002 - Extract Method
Context
Our code should be dirty when programming and writing test cases fast.
After we have good coverage we need to refactor and reduce methods.
Sample Code
Wrong
<?
function retrieveImagesFrom(array $imageUrls) {
foreach ($imageUrls as $index => $imageFilename) {
$imageName = $imageNames[$index];
$fullImageName = $this->directory() . "\\" . $imageFilename;
if (!file_exists($fullImageName)) {
if (str_starts_with($imageFilename, 'https://cdn.example.com/')) {
// TODO: Remove Hardcode
$url = $imageFilename;
// This variable duplication is not really necessary
// When we scope variables
$saveto= "c:\\temp"."\\".basename($imageFilename);
// TODO: Remove Hardcode
$ch = curl_init ($url);
curl_setopt($ch, CURLOPT_HEADER, 0);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
$raw = curl_exec($ch);
curl_close ($ch);
if(file_exists($saveto)){
unlink($saveto);
}
$fp = fopen($saveto,'x');
fwrite($fp, $raw);
fclose($fp);
$sha1 = sha1_file($saveto);
$found = false;
$files = array_diff(scandir($this->directory()), array('.', '..'));
foreach ($files as $file){
if ($sha1 == sha1_file($this->directory()."\\".$file)) {
$images[$imageName]['remote'] = $imageFilename;
$images[$imageName]['local'] = $file;
$imageFilename = $file;
$found = true;
// Iteration keeps going on even after we found it
}
}
if (!$found){
throw new \Exception('We couldnt find image');
}
// Debugging at this point our context is polluted with variables
// from previous executions no longer needed
// for example: the curl handler
}
Right
<?php
function retrieveImagesFrom(string imageUrls) {
foreach ($imageUrls as $index => $imageFilename) {
$imageName = $imageNames[$index];
$fullImageName = $this->directory() . "\\" . $imageFilename;
if (!file_exists($fullImageName)) {
if ($this->isRemoteFileName($imageFilename)) {
$temporaryFilename = $this->temporaryLocalPlaceFor($imageFilename);
$this->retrieveFileAndSaveIt($imageFilename, $temporaryFilename);
$localFileSha1 = sha1_file($temporaryFilename);
list($found, $images, $imageFilename) = $this->tryToFindFile($localFileSha1, $imageFilename, $images, $imageName);
if (!$found) {
throw new \Exception('File not found locally ('.$imageFilename.'). Need to retrieve it and store it');
}
} else {
throw new \Exception('Image does not exist on directory ' . $fullImageName);
}
}
Detection
- [x]Automatic
Most Linters can suggest use for long methods.
This warning also hints us to break and scope our variables.
- Bloaters
Conclusion
Extract Method is our best friend.
We should use it a lot.
Relations
Code Smell 03 - Functions Are Too Long
Code Smell 107 - Variables Reuse
Code Smell 62 - Flag Variables
Credits
Photo by Dustan Woodhouse on Unsplash
Temporary variables can be a problem. They are only useful within their own routine, and therefore they encourage long, complex routines.
Martin Fowler
Software Engineering Great Quotes
Code Smell 155 - Multiple Promises
You have promises. You need to wait. Wait for them all
TL;DR: Don't block yourself in a sorted way.
Problems
- Indeterminism
- Performance bottleneck
Solutions
- Wait for all promises at once.
Context
We heard about semaphores while studying Operating Systems.
We should wait until all conditions are met no matter the ordering.
Sample Code
Wrong
async fetchOne() { /* long task */ }
async fetchTwo() { /* another long task */ }
async fetchAll() {
let res1 = await this.fetchOne();
let res2 = await this.fetchTwo();
// they can run in parallel !!
}
Right
async fetchOne() { /* long task */ }
async fetchTwo() { /* another long task */ }
async fetchAll() {
let [res3, res4] = await Promise.all([this.fetchOne(), this.fetchTwo()]);
//We wait until ALL are done
}
Detection
- [x]Semi-Automatic
This is a semantic smell.
We can tell our linters to find some patterns related to promises waiting.
- Performance
Conclusion
We need to be as close as possible to real-world business rules.
If the rule states we need to wait for ALL operations, we should not force a particular order.
Credits
Thanks for the idea
Photo by Alvin Mahmudov on Unsplash
JavaScript is the only language that I'm aware of that people feel they don't need to learn before they start using it.
Douglas Crockford
Software Engineering Great Quotes
Next article: 5 more code smells.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK