0
\$\begingroup\$

I'm really new to code optimization and JS. And I like to learn with clean code. I would like to ask for help on optimizing my code.

The code below parses the value of four inputs as floating point numbers and adds them together. The sum is then displayed in an output element.

  if(document.getElementById('salary-field').value ===''){
    salary = 0;
   }else{
    salary = parseFloat(document.getElementById('salary-field').value).toFixed(2);
    document.getElementById('salary-field').value = salary;
   } 
    
   if(document.getElementById('commission-field').value ===''){
    commission = 0;
   }else{
    commission = parseFloat(document.getElementById('commission-field').value).toFixed(2);
    document.getElementById('commission-field').value = commission;
   } 
   
   if(document.getElementById('rental-field').value ===''){
    rental = 0;
   }else{
     rental = parseFloat(document.getElementById('rental-field').value).toFixed(2);
     document.getElementById('rental-field').value = rental;
   } 
    
   if(document.getElementById('other-income-field').value ===''){
    other_income = 0;
   } else {
     other_income = parseFloat(document.getElementById('other-income-field').value).toFixed(2);
     document.getElementById('other-income-field').value = other_income;
   }
  
    sum = parseFloat(salary) + parseFloat(commission) + parseFloat(rental) + parseFloat(other_income);
    document.getElementById('sum-field').value = sum.toFixed(2);
\$\endgroup\$
3
  • 2
    \$\begingroup\$ @konijn - I don't remember if I had seen this relevant meta post until now: Do we need "acronym tags" ([dry], [solid], etc.)? \$\endgroup\$ Sep 22, 2021 at 16:44
  • 1
    \$\begingroup\$ I don't know type handling in JavaScript, but the terms in the sum seem to use results of parseFloat() as arguments to parseFloat(). \$\endgroup\$
    – greybeard
    Sep 23, 2021 at 9:36
  • 1
    \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Sep 24, 2021 at 17:01

1 Answer 1

2
\$\begingroup\$

From a short review;

  • You should declare your variables with const or let

  • There is a ton of copy-pasted code in there

  • 'commission-field' is not a great name for your element, I would just call it commission

  • Avoid writing multiple calls to document.getElementById for the same id

  • Its hard to control what users enter, and sometimes parseFloat returns a NaN, I would always deal with that pro-actively

  • I would create a function that formats an element like

    function cleanAmountElement(id){
      const elementValue = document.getElementById(id);
      const cleanerValue = parseFloat(elementValue*1).toFixed(2);
      const cleanestValue = IsNaN(cleanerValue)?0:cleanerValue;    
      document.getElementById(id).value = cleanestValue;
      return cleanestValue;
    } 
    
  • Note that this is not super super clean, that function both updates the UI and returns a value :/

  • Then you can do something funky with functional programming where you pass a list of ids and process them

    const sum = ['salary', 'commission', 'rental', 'other_income'].map(cleanAmountElement)
                                                                  .reduce(getSum,0);
    
    function getSum(a,b){
      return a+b;
    }
    
\$\endgroup\$
2
  • 2
    \$\begingroup\$ inputs have valueAsNumber which coerce empty string into 0. \$\endgroup\$
    – morbusg
    Sep 24, 2021 at 10:41
  • \$\begingroup\$ That comment is part of why I love CR ;) \$\endgroup\$
    – konijn
    Sep 25, 2021 at 8:43

Your Answer

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

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