DEV Community

Cover image for 5 easy wins to refactor even the ugliest code

5 easy wins to refactor even the ugliest code

Mikhail Levkovsky on August 03, 2019

Writing clean code can be a challenge when you start on a new project. Trying to clean up code in an already existing application without breaking ...
Read full post
Collapse
 
gtanyware profile image
Graham Trott • • Edited

If switch is implemented by the compiler as a look-up table it might offer somewhat better performance, especially as the number of options grows. Actually, I often prefer switch to chained ifs as to me it's more aesthetic; it's saying clearly "Here's a set of options to choose from". You can still implement return instead of break in any of the cases. But I guess a lot of things are down to personal preference.

Whenever I see multiple choices of this kind I like to use tables, so I'd try something like this, which also permits easy initialization from JSON (warning: untested code):


const images = {
   "hiphop": "https://unsplash.com/photos/Qcl98B8Bk3I",
   "jazz": "https://unsplash.com/photos/dBWvUqBoOU8",
   "rap": "https://unsplash.com/photos/auq_QbyIA34",
   "country": "https://unsplash.com/photos/RnFgs90NEHY"
};

const url = images[track.getGenre()];
return {
   dimension: 'small',
   url: url ? url : 'https://unsplash.com/photos/PDX_a_82obo'
};
Enter fullscreen mode Exit fullscreen mode
Collapse
 
mlevkov profile image
Mikhail Levkovsky •

Great point! This was just to illustrate an example but yours illustrates it even better :)
Thanks for sharing it!
To be honest, I would prefer to have a designated factory to create these things so as not to mix behavioural and creational logic in one class.

Collapse
 
gtanyware profile image
Graham Trott •

I'd agree with you there Mikhail. It makes the overall program so much more concise and readable.

One of the joys (and frustrations) of JavaScript is there are so many different ways of doing the same thing. As I typed the words "look-up table" I suddenly recognized a pattern, where all the text could be extracted into constant data separate from the behavior. Sometimes you approach these things sideways and the pattern leaps out at you from nowhere.

Collapse
 
juanjoms profile image
Juan Miramontes •

if I were to refactor the getName function, I would simply do:

public getName(): string {
  return this.name ? this.name : this.NAME_DEFAULT;
}
Enter fullscreen mode Exit fullscreen mode

or even:

public getName(): string {
  return this.name || this.NAME_DEFAULT;
}
Enter fullscreen mode Exit fullscreen mode

It includes points 1) and 2), and I think is simpler and more readable.

Collapse
 
nssimeonov profile image
Templar++ •

I don't understand what's wrong with the switch statement? Instead of assigning the variable you could have return for each case and this will also save you the break statement as well.

getBackgroundArt(track: Track): BackgroundImage {
 let backgroundImage: BackgroundImage;
 switch(track.getGenre()) {
   case "hiphop":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/Qcl98B8Bk3I'};
   case "jazz":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/dBWvUqBoOU8'};
   case "rap":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/auq_QbyIA34'};
   case "country":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/RnFgs90NEHY'};
   default:
     return {dimension: 'small', url : 'https://unsplash.com/photos/PDX_a_82obo'};
 }
}

Moreover instead of calling track.getGenre() once, you do that multiple times, and functions may implement complex logic.

Collapse
 
wstone profile image
Will Stone •

Was going to comment with the same. You can even ditch line 2 let backgroundImage: BackgroundImage; now this variable isn't used.

Collapse
 
nssimeonov profile image
Templar++ • • Edited

Yep, forgot that one.. lint usually reminds me in this case :)

Collapse
 
mlevkov profile image
Mikhail Levkovsky •

100% agreed about calling track.getGenre() once, should definetely do that.
as for that last example, that's actually the good one, where you return from each case, which IMO helps keep the flow of the code linear and simple

Collapse
 
jriceindustries profile image
John • • Edited

Number 5 - assign track.getGenre() to a string once at the beginning and compare to that variable.

var genre = track.getGenre();
if (genre == "jazz"){}
...

No need to run the overhead of whatever logic is involved in getGenre() in each if.

Collapse
 
mlevkov profile image
Mikhail Levkovsky •

good call! thanks for spotting it :)

Collapse
 
cubiclebuddha profile image
Cubicle Buddha •

Hypothetical question: What happens if you add a new genre and you don’t want the default banner to be returned?

What I’m getting at is that it’s much safer to throw in a default. Or if it’s typescript it’s safer to return a never type. That way you can exhaustively consider enum values or any union of types. I describe that technique here.

Collapse
 
mlevkov profile image
Mikhail Levkovsky •

Good point, for sure a switch statement should have a default.
I didn't include it in this case for brevity, and this is just an example. For something slightly more evolved I would actually resort to the factory pattern to create my return types and simplify the creation logic even more

Collapse
 
nicoespeon profile image
Nicolas Carlo •

Thank you for this post.
It sums up very nice, simple advices to make code cleaner 👌

Inline Variable (related to #4) and Extract Variable (related to #2) are the two refactorings I'm probably using the most, on a daily basis.

I'm currently working on a VS Code Extension to get the editor do these refactorings automatically for us. My goal is to make these typical operations easy to use.

For example, I can now solve #5 very quickly with Remove Redundant Else.

Maybe you'd be interested in using it.
It's called Abracadabra: bit.ly/vscode-abracadabra

If you feel like trying it, I'd love to get your feedbacks, so I can make it better.
Thanks again for sharing these practices. Have a nice day 😉