How I used "every" to clean up my code

I'm writing some code and the handy tool from CodeClimate, let me know that I have too many return values.

I intentionally did the multiple return values to make the code easier to read but I agree, too many return values.

A clean and simple way to fix that is to change it over to an "every" method.

I put my conditions in to an array and with the assumption that all should return true.

const shouldInitialize = (moreForPromotionService) => {
    const urlSearchParams = new URLSearchParams(window.location.search);

    const params = Object.fromEntries(urlSearchParams.entries());

    if (params.triggerMoreForPromotionsService) {
        return true;
    }

    if (window?.isChina) {
        return false;
    }

    if (window?.isEurope) {
        return false;
    }

    return  !!document.querySelector(moreForPromotionService.mountingPoint));

Here it is after my refactor.

const shouldInitialize = () => {
    const urlSearchParams = new URLSearchParams(window.location.search);

    const params = Object.fromEntries(urlSearchParams.entries());

    const conditions = [
        (
            !!params.triggerMoreForPromotionsService
            || !!document.querySelector(MoreForPromotionsService.mountingPointInit)
        ),
        !window?.isChina,
        !window?.isEurope,
    ];

    return conditions.every((condition) => condition);
};

What are you thoughts? Better solution, please let’s see it!

Comments

[Alex Lohr]

The functionality between the original code and the refactored one differs. The refactored version is needlessly complex and less performant.

The original code had the following decision tree:

Return true if

• if the "triggerMoreForMotionService" search param is set or
• if both
  • window.isChina and
  • window.isEurope is true and
• a mount point can be found in the document.

Otherwise return false.

The decision tree for the refactored version looks like this:

Return true if

• either the search parameter is set or a mount point is in the document and
• window.isChina and window.isInEurope is both false.

Otherwise return false.

Either way, this allows us to restructure the original conditions into a single return statement

const shouldInitialize = (moreForPromotionService) => {
  const urlSearchParams = new URLSearchParams(window.location.search);
  const params = Object.fromEntries(urlSearchParams.entries());

  return !!params.triggerMoreForPromotionsService || (
    !window?.isChina &&
    !window?.isEurope &&
    !!document.querySelector(moreForPromotionService.mountingPoint)
  );
}

And the refactored decision tree:

const shouldInitialize = (moreForPromotionService) => {
  const urlSearchParams = new URLSearchParams(window.location.search);
  const params = Object.fromEntries(urlSearchParams.entries());

  return (
    !!params.triggerMoreForPromotionsService ||
    !!document.querySelector(moreForPromotionService.mountingPoint);
  ) && !window?.isChina && !window?.isEurope;
}

How do you like it?

If you want my advice:

1. Avoid clever code - unless it gives you an edge that comopunds the disadvantages. Clever code adds unneccessary complexity - either directly in the code or for the understanding of the reader and is therefore less maintainable and/or performant.
2. Take your time to understand the problem before you start coding. That your original and refactored code do different things tell me that you did not take enough time to understand your actual requirements, i.e. write down the decision tree - instead you just started coding. Time used to understand your problem before you start coding is never wasted. If in doubt, sit down with the product owner and work out the specifics together.

[DeChamp]

Well you’re right! I was caught in a rush and it shows. I appreciate an in depth walk through.