Probably the hottest code refactoring you ever saw 🔥

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, it's not a problem.

Whilst doing that learned how and why things work in JavaScript.

I never settle, even if something works.

Everything matters, function names, variable names, even prop sorting.

I was doing a code review and reached to a function.

I need to mention here that refactoring took me no more than five minutes.

The function was responsible for formatting a given address depending on its country code.

E.g.

const address = {
  city: "Belgreen",
  countryCode: 'us',
  county: null,
  state: "Vermont",
  suburb: null,
  zip: "4636"
}

The implementation was the following.

const addressTextView = () => {
  const city = address.city;
  const state = address.state;
  const zip = address.zip;
  const suburb = address.suburb;
  const county = address.county;

  switch (address.countryCode) {
    case 'uk':
      return (
        (zip != null ? zip + ', ' : '') +
        (suburb != null ? suburb + ', ' : '') +
        (city != null ? city + ', ' : '') +
        (state != null ? state : '')
      );
    case 'us':
      return (
        (city != null ? city + ', ' : '') +
        (state != null ? state + ', ' : '') +
        (zip != null ? zip : '')
      );
    case 'nz':
      return (
        (zip != null ? zip + ', ' : '') +
        (city != null ? city + ', ' : '') +
        (county != null ? county + ', ' : '') +
        (state != null ? state : '')
      );
    default:
      return (
        (zip != null ? zip + ', ' : '') +
        (suburb != null ? suburb + ', ' : '') +
        (state != null ? state + ', ' : '') +
        (county != null ? county : '')
      );
  }
}

The first thing that bothered me was the ternaries in each case.

Then the repetition.

I started thinking of every return as an array of keys in different order.

I didn't care about null values.

I just started creating patterns.

//
  switch (address.countryCode) {
    case 'uk':
      return [zip, suburb, city, state].join(', ');
    case 'us':
      return [city, state, zip].join(', ');
    case 'nz':
      return [zip, city, county, state].join(', ');
    default:
      return [zip, suburb, state, county].join(', ');
//

Then, I saw the pattern and I created a function to handle each return.

const joinStrings = (...args) => args.join(', ');

And the switch looked like this.

//
  switch (address.countryCode) {
    case 'uk':
      return joinStrings(zip, suburb, city, state);
    case 'us':
      return joinStrings(city, state, zip);
    case 'nz':
      return joinStrings(zip, city, county, state);
    default:
      return joinStrings(zip, suburb, state, county);
//

Then I did something that amazed some people.

The way I filtered out null values from each array.

const joinStrings = (...args) => args.filter(Boolean).join(', ');

And final code was this.

const joinStrings = (...args) => args.filter(Boolean).join(', ')

const formatAddress = ({ city, county, countryCode, state, suburb, zip }) => { 
  switch (countryCode) {
    case 'uk':
      return joinStrings(zip, suburb, city, state);
    case 'us':
      return joinStrings(city, state, zip);
    case 'nz':
      return joinStrings(zip, city, county, state);
    default:
      return joinStrings(zip, suburb, state, county);
  }
};

My thoughts.

Both functions work. Business is happy.

It's OK to deliver it, but...

always try to improve and never settle if something works.

We had a function called addressTextView which was not very clear what it does. It also used the address object from the parent scope.

Then we had a lot of logic with ternaries which where not very clear to read at first either.

I renamed the function to formatAddress to be clear and passed the address object as an argument.

I isolated the logic to a different function named joinStrings. That function is independent from formatAddress and can be reused if necessary.

We also went from 45 lines of code down to 13. 😍

That's it.

I am not bragging, I'm trying to say it doesn't matter if it works but if you want to learn and grow as a developer there are a lot of ways to do that.

Comments

[DSI Developer]

How about destructuring the address into all the variables?

[Ioannis Potouridis]

What about it? 😀

[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.

[Ioannis Potouridis]

Great! Thanks for sharing.

[pacozaa]

Damn. The join string function is 🔥

Love this kind of stuff.

[JWP]

Excellent post, thanks.

[Tal Hayut]

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

Literally had to 😂

[Shalvah]

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

[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(', ');
};

[AncientSwordRage]

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

[Kamal]

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

[Faiz Shahiran]

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

[Alan]

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.

[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.

[Tomaz Lemos]

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

[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...

[Ioannis Potouridis]

Thanks for sharing this. 🙏

[Internet of Brewing]

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

[Cody Crumrine]

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. 👍