3
\$\begingroup\$

I´m a programing rookie with minor javascript skills. So I ask you for a code review since I'm not sure whether my code is ok.

Short explanation of my program: I work for the heating industry. Radiators have a different heat output. The standard heat output is calculated with:

  • heat flow = 75°C
  • heat return = 65°C
  • heat temperature = 20°C
  • a specific heat exponent

My program can calculate the heat output with different values e.g.

  • heat flow = 60°C
  • heat return = 55°C
  • heat temperature = 18°C
  • a specific heat exponent

There are restrictions:

  • Heat return must not be higher than heat flow
  • Heat temp must not be higher than heat flow and heat return
  • Input field must not be empty

"use strict";
const heatFlowCalculator = {
    init: function() {
        this.setEventHandler();
    },

    getDom: {
        heatFlow: document.getElementById('heatFlow'),
        heatReturn: document.getElementById('heatReturn'),
        heatTemp: document.getElementById('heatTemp'),
        heatValue: document.getElementById('heatValue'),
        heatExponent: document.getElementById('heatExponent'),
        buttonResult: document.getElementById('buttonResult'),
        renderResult: document.getElementById('renderResult')
    },

    getFixedValue: function() {

        const normHeatFlow = 75,
              normHeatReturn = 65,
              normHeatTemp = 20,
              // formula for fixed value  
              result = (normHeatFlow - normHeatReturn) / 
                       (Math.log((normHeatFlow - normHeatTemp) / 
                       (normHeatReturn - normHeatTemp)));       

        return result;        
    },
    /* 
    Check if the user Input is valid.
    Case 1: (isSmallerThan): heatFlow value has to be greater than heatReturn value
            heatTemp has to be smaller than heatFlow value or heatReturn value
    Case 2: (isNotEmpty): check if the input fields have values
    
    If both cases return true, the input is valid! 
    */
    isInputValid: function() {

        const heatFlow = this.getDom.heatFlow.value,
              heatReturn = this.getDom.heatReturn.value,
              heatTemp = this.getDom.heatTemp.value,
              heatValue = this.getDom.heatValue.value,
              heatExponent = this.getDom.heatExponent.value;

              function isSmallerThan() {
                return (heatReturn < heatFlow &&
                       heatTemp < heatFlow &&
                       heatTemp < heatReturn)
              }

              function isNotEmpty() {
                  return (
                         heatFlow !== "" || 
                         heatReturn !== "" || 
                         heatTemp !== "" || 
                         heatValue !== "" || 
                         heatExponent !== ""
                         ) ? true : false;  
              }

        return isSmallerThan() && isNotEmpty();
    },

    getTempDifference: function() {
    
        const heatFlow = this.getDom.heatFlow.value,
              heatReturn = this.getDom.heatReturn.value,
              heatTemp = this.getDom.heatTemp.value,
              // Formula for heat difference  
              result = (heatFlow - heatReturn) / 
                       (Math.log((heatFlow - heatTemp) / 
                       (heatReturn - heatTemp)));

        return result;        
    },

    setResult: function() {

        const heatValue = this.getDom.heatValue.value,
              heatExponent = this.getDom.heatExponent.value,
              tempDifference = this.getTempDifference(),
              fixedValue = this.getFixedValue(),
              // formula to get the final result  
              result = Math.round(heatValue * Math.exp(
                       (Math.log(tempDifference / fixedValue)) 
                       * heatExponent));
                       
        if (this.isInputValid()) {
            return `The calculated value is ${result}`;
        } else {
            return `The input is invalid! `;
        } 
    },
    // render the result of setResult() to the DOM
    showResult: function() {
        const showResult = this.getDom.renderResult;
        showResult.textContent = this.setResult();
    },

    setEventHandler: function() {
        const _this = this,
              button = this.getDom.buttonResult;

        button.addEventListener('click', function() {
            _this.showResult();
        }, false);
    }
};

heatFlowCalculator.init();
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="X-UA-Compatible" content="ie=edge">
    <title>Document</title>
    <style>
        input {
            display: block;
        }
        #renderResult {
            margin-top: 25px;
        }
    </style>
</head>
<body>
    Heat flow<input type="number" name="HeatFlow" id="heatFlow" placeholder="70">
    Heat return<input type="number" name="HeatReturn" id="heatReturn" placeholder="60">
    Heat temperature<input type="number" name="HeatTemp" id="heatTemp" placeholder="20">
    Heat value<input type="number" name="HeatValue" id="heatValue" placeholder="2100">
    Heat exponent<input type="number" name="HeatExponent" id="heatExponent" placeholder="1.3211">
    <br>
    <button id="buttonResult">Calculate</button>
    <div id="renderResult"></div>
    
    <script src="heatflow.js"></script>
</body>
</html>

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

The flow of this app is,

  1. Get the input values
  2. Check if the input values are valid
  3. Calculate,output the value or "invalid"

Since you use const,I assume you understand/prefer es6,arrow function,etc...

For 1. ,you can simplify your getDom()

getDom: selector=>document.getElementById(selector)
//same as
//getDom: function (selector){return document.getElementById(selector)}

// usage
const heatFlow = this.getDom('heatFlow').value

You concern to get values here,you can have a function just to get all the values you need.

getAllValues:()=>{
  const getValue=name=> this.getDom(name).value;
  const heatFlow=getValue("heatFlow");
  const heatReturn=getValue("heatReturn");
  return {heatFlow, heatReturn,.........}
  //same as return {heatFlow:getValue("heatFlow'),.....}
}
//when you need to get the values by const heatFlow=.....,instead
const {heatFlow, heatReturn, heatTemp, heatValue}=this.getAllValues();
// this is Object Destruction
// you get all the values you want,try console.log(heatFlow, heatReturn, heatTemp, heatValue)
// in case you want to rename heatFlow to normHeatFlow.
// const {heatFlow:normHeatFlow}=this.getAllValues();

For 2. , to check if input is valid,in isNotEmpty(),

return (
heatFlow !== "" || 
heatReturn !== "" || 
heatTemp !== "" || 
heatValue !== "" || 
heatExponent !== ""
) ? true : false;  

A mistake here you used ||(OR),which means either one of the input is not empty,it will pass.Instead you need &&(AND).

Also,the following comprasion is the same.

heatFlow !== "" ? true : false;
heatFlow !== ""

So you only need to

return (
heatFlow !== "" && 
heatReturn !== "" && 
heatTemp !== "" && 
heatValue !== "" && 
heatExponent !== ""
)

You made meaningful names for isSmallerThan() && isNotEmpty() which is good.

And the rest in coding part is fine.

In aspect of design, one thing to notice about the placeholder. As you placed numbers on it,I thought the input are pre-filled with numbers as an example.I also don't familar what will it do,it output invalid and I still thought I have put the numbers. Consider to put "type 70" in placeholder,or just put actual numbers in the inputs.

Below is additional part,you can learn or skip.

For coding,

return (
heatFlow !== "" && 
heatReturn !== "" && 
heatTemp !== "" && 
heatValue !== "" && 
heatExponent !== ""
)

If you have more condition to compare,you can use Array.every().

const values=[heatFlow, heatReturn, heatTemp, heatValue, heatExponent];
const isNotEmpty=value=> value!==""
return values.every(isNotEmpty);

So you only need to write !=="" once.

For design,there's a function onChange can be put in the input.Everytime a user changed the value,it triggers. You can use it to auto calculate the output,rather than chaning value and click button Calculate.But I don't know your intention of this behind,it could be dangeous or improper to change the heat easyily,so it's up to you.

\$\endgroup\$
1
\$\begingroup\$

I agree with most of the points in the answer by sbk201 - especially about the placeholder attributes. Given that those are mostly values that would fit for the <input> elements, it would be fitting just to use the value attribute to set initial values instead of using placeholders.

Did you consider using an <output> element for the display of the result? Most browsers except IE support it.


The code in setEventHandler() could be simplified from this:

const _this = this,
      button = this.getDom.buttonResult;

button.addEventListener('click', function() { 
    _this.showResult();
}, false);

To a single line that uses a function reference:

this.getDom.buttonResult.addEventListener('click', this.showResult, false);

If you needed to have multiple lines in that anonymous function, then an arrow function would eliminate the need for _this because there is no separate this context. Alternatively, if you wanted to use the traditional function statement syntax, you could also utilize Function.bind() to bind the context to this instead of assigning _this to this beforehand.

\$\endgroup\$

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.