DEV Community

Probably the hottest code refactoring you ever saw 🔥

Ioannis Potouridis on January 04, 2020

I have some beliefs on programming. I always re-write, re-think, research, re-invent and refactor. That did cost me time in the beginning but now...
Collapse
 
bsaad profile image
bsaad

Great post, I like how you took the time to explain step by step.

I'd suggest also the following change to make the "business rules" clearer by extracting them from the formatting logic:

const formatMap = {
  uk: ["zip", "suburb", "city", "state"],
  us: ["city", "state", "zip"],
  nz: ["zip", "city", "county", "state"],
  default: ["zip", "suburb", "state", "county"],
}

const formatAddress = (address) => { 
  const format = formatMap[address.countryCode] || formatMap.default;
  return format.map(field => address[field]).filter(Boolean).join(', ');
};
Collapse
 
kelerchian profile image
Alan • Edited

Unless formatMap is something that needs to be dynamic, I suggest not to do this.

This mixes up type and data and also smells like premature abstraction.

Collapse
 
kamalhm profile image
Kamal

this is also what i like to do whenever a switch case appears

Collapse
 
tomazfernandes profile image
Tomaz Lemos

Actually I’ve just written a post about this kind of pattern, if you’d like to take a look:
dev.to/tomazlemos/keeping-your-cod...

Collapse
 
potouridisio profile image
Ioannis Potouridis

Thanks for sharing this. 🙏

Collapse
 
potouridisio profile image
Ioannis Potouridis

Well one way or another the formatAddress function needs to know the business rules. If I went with your approach I would pass the rules as an argument.

Collapse
 
ancientswordrage profile image
AncientSwordRage

This is the final image in a magneto_perfection.jog meme. I love it.

Collapse
 
faezshaeran profile image
Faiz Shahiran

this is nice.. considering formatMap value can be saved in config file or database. Its easier to scale.

Collapse
 
tomazfernandes profile image
Tomaz Lemos

I’d never seen this pattern in JS, looks just as nice as in Java 👍🏼👍🏼

Collapse
 
aleksikauppila profile image
Aleksi Kauppila

Thanks for posting Ioannis, good job!

You can take this to the next level with Replace conditional with Polymorphism. Even though you have successfully improved the code in this example, every new format will require you to modify this algorithm.

Collapse
 
potouridisio profile image
Ioannis Potouridis

Great! Thanks for sharing.

Collapse
 
tool3 profile image
Tal Hayut

const { city, state, zip, suburb, county } = address;

Literally had to 😂

Collapse
 
xdega profile image
Liam Hockley

I gotta say. The refactor looks good, but the lack of consistency in the order of your arguments really triggers me.

While you are mapping the args, the order doesn't matter in terms of working code, but it could be a wee bit more readable to the human eyes that work with the code. ;)

Collapse
 
potouridisio profile image
Ioannis Potouridis • Edited

Hmm, I don't know if you're talking about joinStrings function which in this case the order of the arguments does matter. On the other hand, I could simply pass a single array argument which was my first attempt.

Collapse
 
the3rdc profile image
Cody Crumrine • Edited

I like the refactor in general, but I'm not 100% sure I'm on board with the joinStrings function as opposed to a .join().filter() in each switch case. Seems like maybe more a case of abstraction for abstraction's sake than valuable code reuse.

I'd also maybe prefer a more explicit null check (still using the filter method) for clarity sake. But that may be just be preference.

All that aside, the simplification from all the conditional checks and concatenations to a single join and filter of an array is spot on. 👍

Collapse
 
muramasah profile image
Felipe

Hi Ioannis, I think you cleaned very well the code, but it is still rigid an fragile. For example, try to change the way the message is formated in some countries, like using dots for some properties instead of comas, or try to add a new field to every country. In every case you must change the written code breaking with the SOLID's open-closed principle.

The source of evil here is the switch, your function formatAddress has two responsabilities, (1) to determine what country code has the adress object, and (2) choose the properties to be passed to joinStrings for each case.

An easy pattern that bring us some flexibility in these cases is the chain of responsabilities. If you dislike this approach you have others more functional but the responsabilities coupled there are a big issue from my point of view, because limits the extention of features that has a lot of probabilities to change.

Btw, all of I'm saying here do not take away the fact that you have done an awesome job until here!

Sorry for the bad english,
Have fun!

Collapse
 
potouridisio profile image
Ioannis Potouridis

You're totally correct.

The format and number of countries we serve is fixed so I didn't take into consideration these principles. I should but this was a function with a specific purpose and posted this as an example for cleaner code. Thank you for your feedback and don't worry for your English. 😀

Collapse
 
brewnode profile image
Internet of Brewing

Code is art! Keep refactoring, striving for beauty. Simplify as much as possible but no more.

Collapse
 
zpwebbear profile image
Dmitriy Braginets

Beautiful solution ;)

Collapse
 
jwp profile image
John Peters

Excellent post, thanks.

Collapse
 
angryziber profile image
Anton Keks

Please remember that zip is called "postal code" everywhere outside of US

Collapse
 
potouridisio profile image
Ioannis Potouridis

Tell that to my back end! 😍

Collapse
 
shalvah profile image
Shalvah

Thought this was clickbait at first, but this was a genuinely good refactoring, one that people can't argue with. Nice!

Collapse
 
pacozaa profile image
pacozaa

Damn. The join string function is 🔥

Love this kind of stuff.

Collapse
 
dsidev profile image
DSI Developer

How about destructuring the address into all the variables?

Collapse
 
potouridisio profile image
Ioannis Potouridis

What about it? 😀

Collapse
 
rhysbrettbowen profile image
Rhys Brett-Bowen • Edited

The first code you could end up with a comma and space at the end of a string (i.e. Case is uk and state is null). Join doesn't have this issue so the refactor also fixed the big.

Collapse
 
potouridisio profile image
Ioannis Potouridis

Awesome, didn't notice that! 😂