DEV Community

Discussion on: Code Smell 154 - Too Many Variables

Collapse
 
joolsmcfly profile image
Julien Dephix

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');
Enter fullscreen mode Exit fullscreen mode

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];
Enter fullscreen mode Exit fullscreen mode

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

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

Something like:

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

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

Collapse
 
mcsee profile image
Maxi Contieri

Hi

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

More on this here

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