DEV Community

The Art of Refactoring: 5 tips to Write Better Code

Nya on October 10, 2019

Bad code works. We all know this. Developers have been writing code for years without giving a single thought to whether they were doing it right...

Read full post

Daniel Worsnup • Oct 10 '19

Great article! An important thing to keep in mind when using objects as lookup tables is that objects have prototype chains that will also be checked if you are not careful. Look at this example, using some code from the article:

There are a few fixes for this:

Create the object without a prototype (Object.create(null)):

const pokemon = Object.assign(Object.create(null), {
    Water: 'Squirtle',
    Fire: 'Charmander',
    Plant: 'Bulbasur',
    Electric: 'Pikachu'
});
// pokemon.toString does not exist!

Only check the object's "own" properties (hasOwnProperty)

function getPokemon(type) {
  return pokemon.hasOwnProperty(type) ? pokemon[type] : 'Mew';
}

As you also suggested: Use a Map which inherently doesn't have this problem.

Thanks for the content!

oshell • Oct 10 '19

The example with the switch statement sounds cool first, but is far from any real world use case. Yes, if all I do with the switch is checking a string I can move it to an object. But normally I check against boolean values, returned from several function calls. Maybe you should mention that.

In my opinion the most important thing is to have readable statements. Instead of putting multiple values into the if statement, define a new variable describing what this statement contains. You sacrifice minimal resources for much more readability. No comments needed. E.g.:

const pictureIsValidForUpload = validWidth && validExtension && validMimeType;
If (pictureIsValidForUpload) { upload();}

Daniel Calderon • Dec 3 '19 • Edited

I'm not sure if you need a switch statement to check a boolean in first place.

The object alternative to the switch statement could work when you have different conditions to be checked.

E.g:

let type = 0;
const objContent = {
0: 'first',
1: 'second',
2: 'test',
23: Michael''
}

content = objContent[type]

This is easier to read than a switch statement or even a regular if

if(type === 0) return "first"
else if(type === 1)...etc

Patrick Tingen • Oct 11 '19

Note that there is some debate wether you should be DRY (dont repeat yourself) or WET (write everything twice). Blindy pushing code to functions the second time you encounter it may not always be the best solution.

In addition: when you /do/ move it to one function, make really sure that the intention behind it is the same as well. You might end up changing the one function, only to discover that the change is only valid for one of the cases where it is called.

Denis Howe • Oct 11 '19

Can you point to any convincing argument in favour of WET?

My most convincing example in favour of DRY was fixing a bug then discovering that that code had been duplicated, fixing the copy, then finding that those two bits of code had both been duplicated, and so on. After fixing the same bug in 16 places, I turned them all into calls to one function. So, no, don't write anything twice if you can help it.

Patrick Tingen • Oct 11 '19

Yes, I could, but this post from @xtrasmal does it best. The article that is linked to is well worth reading.

Xander • Dec 19 '18 • Edited

Just wanted to leave this here for people who are interested.

"DRY is about Knowledge, Code duplication is not the issue."
verraes.net/2014/08/dry-is-about-k...

In your example, I believe that when you encounter 16 copies of a snippet, it is fair to say that you did a good job of bringing them together. The point of the article linked is that you need to take good care in deciding if two identical pieces of code really /mean/ the same thing.

Robert Larsson • Oct 12 '19

I have a better suggestion on the news part

function getNewsByType(type) {
  const allNews = getNewsFromWeb();
  return allNews.filter(news => news.type === type);
}

getNewsByType('javascript');
getNewsByType('rust');
getNewsByType('golang');

Magne • Dec 8 '20

you forgot the .map part, so it returns the news content.

Denis Howe • Oct 11 '19

Excellent. If only everyone understood the benefits of guard clauses! Some religions used to preach that a function should only have a single point of return, but that leads to hard-to-understand nested ifs. The other error is blindly going down the "happy path" first and leaving exceptions until later. Far better to test for the exceptional condition first and return or throw an error at that point in the code, allowing the reader forget about it and reducing nesting.

hidden_dude • Oct 14 '19

I think the obvious way to get rid of switch statements and long if/else clauses is with OO:

Instead of asking what "type" something is, use an objet that has all the attributes about that type in it.

so instead of :

getPokemon(type)

You'd write:

pokemon = Pokemon.fromString(typeStr)

name = pokemon.getName()

The obvious advantage is that instead of having a bunch of switch statements in your code, or a bunch of if/elses, or a bunch of tables that trigger on type, you have a single object that encapsulates the entire description of the pokemon, including name, preferences, color, size, etc.. You may have a table (or switch or if/else) to create that object from a string that indicates the "type". Or you could just use the specific class name everywhere (or prototypes if you prefer to do it that way).

That's fairly basic OO, I'm kind of surprised nobody mentioned this.