5
\$\begingroup\$

I'm really new to code optimization and JS. I would like to ask for help on optimizing my code. Thank you in advance.

Here is my code:

if(entity_param_connections_products == "PL") {
      if(pl_flex_l1_label === ""){
        document.getElementById("credit_app-flex_l1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l1-label').innerHTML = pl_flex_l1_label;
      }
      
      if(pl_flex_n1_label === ""){
          document.getElementById("credit_app-flex_n1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n1-label').innerHTML = pl_flex_n1_label;
      }
      
      if(pl_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = pl_flex_t1_label;
      }
      
      if(pl_flex_l2_label === ""){
          document.getElementById("credit_app-flex_l2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l2-label').innerHTML = pl_flex_l2_label;
      }
      
      if(pl_flex_n2_label === ""){
          document.getElementById("credit_app-flex_n2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n2-label').innerHTML = pl_flex_n2_label;
      }
      
      if(pl_flex_t2_label === ""){
          document.getElementById("credit_app-flex_t2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t2-label').innerHTML = pl_flex_t2_label;
      }
  }else if(entity_param_connections_products == "CC"){
      if(cc_flex_l1_label === ""){
        document.getElementById("credit_app-flex_l1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l1-label').innerHTML = cc_flex_l1_label;
      }
      
      if(cc_flex_n1_label === ""){
          document.getElementById("credit_app-flex_n1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n1-label').innerHTML = cc_flex_n1_label;
      }
      
      if(cc_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = cc_flex_t1_label;
      }
      
      if(cc_flex_l2_label === ""){
          document.getElementById("credit_app-flex_l2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l2-label').innerHTML = cc_flex_l2_label;
      }
      
      if(cc_flex_n2_label === ""){
          document.getElementById("credit_app-flex_n2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n2-label').innerHTML = pl_flex_n2_label;
      }
      
      if(cc_flex_t2_label === ""){
          document.getElementById("credit_app-flex_t2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t2-label').innerHTML = cc_flex_t2_label;
      }
  }else if(entity_param_connections_products == "II"){
      if(ii_flex_l1_label === ""){
        document.getElementById("credit_app-flex_l1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l1-label').innerHTML = ii_flex_l1_label;
      }
      
      if(ii_flex_n1_label === ""){
          document.getElementById("credit_app-flex_n1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n1-label').innerHTML = ii_flex_n1_label;
      }
      
      if(ii_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = ii_flex_t1_label;
      }
      
      if(ii_flex_l2_label === ""){
          document.getElementById("credit_app-flex_l2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l2-label').innerHTML = ii_flex_l2_label;
      }
      
      if(ii_flex_n2_label === ""){
          document.getElementById("credit_app-flex_n2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n2-label').innerHTML = ii_flex_n2_label;
      }
      
      if(ii_flex_t2_label === ""){
          document.getElementById("credit_app-flex_t2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t2-label').innerHTML = ii_flex_t2_label;
      }
  }else if(entity_param_connections_products == "EI"){
     if(ei_flex_l1_label === ""){
        document.getElementById("credit_app-flex_l1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l1-label').innerHTML = ei_flex_l1_label;
      }
      
      if(ei_flex_n1_label === ""){
          document.getElementById("credit_app-flex_n1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n1-label').innerHTML = ei_flex_n1_label;
      }
      
      if(ei_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = ei_flex_t1_label;
      }
      
      if(ei_flex_l2_label === ""){
          document.getElementById("credit_app-flex_l2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l2-label').innerHTML = ei_flex_l2_label;
      }
      
      if(ei_flex_n2_label === ""){
          document.getElementById("credit_app-flex_n2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n2-label').innerHTML = ei_flex_n2_label;
      }
      
      if(ei_flex_t2_label === ""){
          document.getElementById("credit_app-flex_t2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t2-label').innerHTML = ei_flex_t2_label;
      }
  }else if(entity_param_connections_products == "OV"){
      if(ov_flex_l1_label === ""){
        document.getElementById("credit_app-flex_l1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l1-label').innerHTML = ov_flex_l1_label;
      }
      
      if(ov_flex_n1_label === ""){
          document.getElementById("credit_app-flex_n1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n1-label').innerHTML = ov_flex_n1_label;
      }
      
      if(ov_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = ov_flex_t1_label;
      }
      
      if(ov_flex_l2_label === ""){
          document.getElementById("credit_app-flex_l2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_l2-label').innerHTML = ov_flex_l2_label;
      }
      
      if(ov_flex_n2_label === ""){
          document.getElementById("credit_app-flex_n2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_n2-label').innerHTML = ov_flex_n2_label;
      }
      
      if(ov_flex_t2_label === ""){
          document.getElementById("credit_app-flex_t2").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t2-label').innerHTML = ov_flex_t2_label;
      }
  }
Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ I'm really new to code optimization Better develop a useful judgement of when to pursue code improvements first. \$\endgroup\$
    – greybeard
    Dec 16, 2021 at 6:30
  • \$\begingroup\$ (I'm afraid this is a beginner's real code.) \$\endgroup\$
    – greybeard
    Dec 16, 2021 at 6:32
  • \$\begingroup\$ We need to know what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task, rather than your concerns about the code. \$\endgroup\$
    – Toby Speight
    Dec 16, 2021 at 13:16

3 Answers 3

Reset to default
6
\$\begingroup\$

As already suggested, you can abstract common logic to a function:

const updateElement = function(idPrefix, value) {
  if (value === "") {
    document.getElementById(idPrefix).style.display = "none";
  } else {
    document.getElementById(idPrefix + "-label").innerHTML = value;
  }
}
updateElement("credit_app-flex_t1", pl_flex_t1_label);

Still, this would lead to duplicated code:

if(entity_param_connections_products === "PL") {
  updateElement("credit_app-flex_t1", pl_flex_t1_label);
  ...
} else if(entity_param_connections_products == "CC") {
  updateElement("credit_app-flex_t1", cc_flex_t1_label);
  ...
}
...

To remove this duplicated code, you could use an object:

const configs = {
  PL: {
    l1: pl_flex_l1_label,
    n2: pl_flex_n2_label,
    ...
  },
  CC: {
    l1: cc_flex_l1_label,
    ...
  },
  ...
};
const config = configs[entity_param_connections_products];
for(const [id, value] of Object.entries(config)) {
  updateElement("credit_app-flex_" + id, value);
}
Improve this answer
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I would also take a good look at the XX_flex_YY_label variables and see if those can be restructured into a flex_label.XX.YY structure \$\endgroup\$
    – Dancrumb
    Aug 21, 2021 at 13:34
  • \$\begingroup\$ @Dancrumb that would be a great idea, as that could make the code even simpler (in my suggestion it would remove the configs object). And we would avoid the idea of accesing the window object with formated variables names (as suggested by @Blindman67) \$\endgroup\$
    – Miguel Alorda
    Aug 22, 2021 at 8:45
2
\$\begingroup\$

D.R.Y.ing code

To remove repeated code

Common unchanging code

Locate the common parts of the repetitive code and build functions that contains the repeated code, we replace the changing part by passing arguments to function with the values that are changing.

Unique code

Next we locate changing parts of the code. We must build names and get values based on the unique values we pass to the functions created in the step above.

In your case

  • Label text has the name eg pl_flex_l1_label that consist of two changing parts that is highlighted with "#" #1_flex_#2_label

    • "#1" is the same as the value of entity_param_connections_products forced to lowercase

    • "#2" is one of 6 values "l1", "n1", "t1", "l2", "n2" and, "t2" that are the same for each of the values for "#1". We can build an array to hold these values.

      const labelTypes = "l1,n1,t1,l2,n2,t2".split(",");

  • There are two elements per label, one is contains that label and the second holds the text. They are identified by id "credit_app-flex_#2" and "credit_app-flex_#2-label". Again "#2" is one of six values common to all values of "#1"

    The next function buildLabel gets a label value from the global object window and the label element id returning it as an array. prod is the value entity_param_connections_products and type is one of the six label types.

    For each label type we call buildLabel and put ir in an array. We do this in the function getProductLabels that returns an array of arrays

    const buildLabel = (prod, type) => [
    window[[prod.toLowerCase(), "flex", type, "label"].join("_")], 
    "credit_app-flex_" + type
];  
const getProductLabels = productCon => labelTypes.map(type => buildLabel(productCon, type));

  • Now we have an array that contains all the unique values associated with a entity_param_connections_products value. The last thing we do is hide or display the associated label for each of the items in the array create by getProductLabels

    The function updateLabel takes an array containing the label value and the label id and hides or displays the label depending on the label value. This is called once per label type.

    const updateLabel = ([label, productId]) => {
      const el = document.getElementById(productId + (label ? "-label" : ""));
      label ? el.textContent = label : el.style.display = "none";
}

Rewrite

The only part that changes at the top level is the value of entity_param_connections_products which we can use to start the chain of calls of the functions we defined above.

We put all the code together in a function that encapsulate all the parts and keeps them safely out of the global scope.

function updateLabels(productName) {
    const labelTypes = "l1,n1,t1,l2,n2,t2".split(",");
    const buildLabel = (prod, type) => [
        window[[prod.toLowerCase(), "flex", type, "label"].join("_")], 
        "credit_app-flex_" + type
    ];
    const getProductLabels = productCon => labelTypes.map(type => buildLabel(productCon, type));
    const updateLabel = ([label, productId]) => {
        const el = document.getElementById(productId + (label ? "-label" : ""));
        label ? el.textContent = label : el.style.display = "none";
    }
    getProductLabels(productName).forEach(updateLabel);
}

With just the above function in place we can now replace all your code with

updateLabels(entity_param_connections_products);

DRY V original

We have taken 180 lines of repetitive code and put it into 14 lines. If names or ids change and the common parts remain the same it is easy to change a value without needing to change each unique value in the code.

Improve this answer
\$\endgroup\$
1
\$\begingroup\$

You can use functions and ternary operator logic here to reduce a lot of line and to make it more clear.

For example

if(pl_flex_t1_label === ""){
          document.getElementById("credit_app-flex_t1").style.display = 'none';
      } else {
          document.getElementById('credit_app-flex_t1-label').innerHTML = pl_flex_t1_label;
      }

can be simplified into

var setStyleDisplayToNon = function(id) {
  document.getElementById(id).style.display = 'none';
};

var setInnerHTML = function(id, value) {
  document.getElementById(id).innerHTML = value;
};

pl_flex_t1_label === "" ? setStyleDisplayToNon("credit_app-flex_t1") : setInnerHTML("credit_app-flex_t1-label", pl_flex_t1_label);

With functions setStyleDisplayToNon and setInnerHTML defined only once, all your code can simply call those functions.

Improve this answer
\$\endgroup\$
1
  • 3
    \$\begingroup\$ This will still leave you with 30 lines of nearly identical code. \$\endgroup\$
    – Teepeemm
    Aug 20, 2021 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.