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

1. Remove commented code
2. 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.

Tags

• 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

1. If you need a temporary hack, make it explicit
2. 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.

Tags

• 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

1. 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.

Tags

• Bloaters
• Naming

Conclusion

There are no hard rules on name length.

Just Heuristics.

Relations

Code Smell 33 - Abbreviations

More Info

• What exactly is a name? — Part I: The Quest
• What exactly is a name? — Part II: Rehab
• Long and Short of Naming

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

1. Extract Method
2. 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.

Tags

• 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

1. 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.

Tags

• 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

Twitter

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.