Maxi Contieri

Posted on Jul 31, 2022 • Originally published at maximilianocontieri.com

Code Smell 154 - Too Many Variables

#webdev #beginners #programming #tutorial

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

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 no really necesary 
          // 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.

Conclusion

Extract Method is our best friend.

We should use it a lot.

Relations

Code Smell 149 - Optional Chaining

Maxi Contieri ・ Jul 16 '22

#javascript #webdev #beginners #programming

Code Smell 107 - Variables Reuse

Maxi Contieri ・ Dec 1 '21

#oop #programming #webdev #tutorial

Code Smell 62 - Flag Variables

Maxi Contieri ・ Feb 8 '21

#programming #oop #webdev #tutorial

Refactorings

Refactoring 002 - Extract Method

Maxi Contieri ・ Nov 25 '21

#refactoring #oop #webdev #codenewbie

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

Maxi Contieri ・ Dec 28 '20

#codenewbie #programming #quotes #software

This article is part of the CodeSmell Series.

How to Find the Stinky parts of your Code

Maxi Contieri ・ May 21 '21

#codenewbie #tutorial #codequality #beginners

Top comments (3)

Maxi Contieri • Aug 1 '22

you are right !
Changed !
thank you

Julien Dephix • Aug 1 '22

There are various things missing or not working in your code example. It doesn’t change the meaning you’re trying to convey, of course, but it makes it look like you rushed and didn’t pay attention to details.

Parameter is string imageUrls (it’s missing the dollar sign) but you iterate over it so it should be an array, not a string.

throw new \Exception('We could'nt find image');

Syntax error: either escape the apostrophe or surround the message with double quotes.

I know it’s not the scope of your post but you could have passed $imageNames as param to avoid referencing a variable outside of the function scope.

$imageName = $imageNames[$index];

Talking about too many variables you can just return a Boolean here:

list($found, $images, $imageFilename) = $this->tryToFindFile($localFileSha1, $imageFilename, $images, $imageName);

Something like:

// method probably doesn’t need all 4 parameters
if (!$this->fileExists($localFileSha1, $imageFilename, $images, $imageName)) {
    // throw exception
}

I like the idea of showcasing code smells but it would look better with working code IMHO.

Maxi Contieri • Aug 1 '22

Thank you very much for taking the time to correct the code.
I've made several of your suggestions.

I treat code as pseudocode and not real one

I Wrote More than 90 Articles on 2021: Here is What I Learned

Maxi Contieri ・ Dec 31 '21 ・ 4 min read

#writing #tutorial #webdev #cleancode

I've also decided not to use 'php list()' returning more than one argument because it is not common on other languages

DEV Community