2
\$\begingroup\$

Finished my first lone project and I'm wondering if the execution is passable. This is a basic lbs to tons converter. The customer drives on the scale loaded, then dumps the load and returns unloaded to the scale again. We need to convert the leftover lbs to tons. I am wondering if this is a concise way to perform this task.

loaded weight - unloaded weight = Total lbs in scrap

total lbs * 0.0005 = weight in tons

function calc() {
    let loadedAmount = document.getElementById("loaded").value;
    let unloadedAmount = document.getElementById("unloaded").value;
    let final = document.getElementById("result").value = 
Math.round(`${(loadedAmount - unloadedAmount) * 0.0005}` * 100)/100; 
    let resultInPounds = document.getElementById("result").value = 
Math.round(`${(loadedAmount - unloadedAmount)}`);

    if (!loadedAmount) {
       document.getElementById("loaded").value=alert("waiting for an initial value for loaded");
    } else {
        loadedAmount = parseInt(loadedAmount);
    }
    if (!unloadedAmount) {
        document.getElementById("unloaded").value = alert("waiting for an initial value for unloaded");
       
    } else {
        unloadedAmount = parseInt(unloadedAmount);
    }
    if (loadedAmount < unloadedAmount) {
        alert("loaded amount must be greater than unloaded amount");
    } 
    console.log(`loadedAmount: ${loadedAmount}`);
    console.log(`unloadedAmount: ${unloadedAmount}`);

    document.getElementById("result").innerHTML=final + " Tons or " + 
resultInPounds + " lbs";

    console.log(result);
    
}
<!DOCTYPE html>
<html>
<head> 
    <meta charset="utf-8">
<title></title>
<style>
   
</style>
</head>
<body>
    <header>Negaunee Iron and Metal</header>
    <script src="app.js"></script>
        <div >input loaded pounds
            <input type="number"  id="loaded" placeholder="Loaded...">
        </div>
        <div > input unloaded pounds
            <input type="number" id="unloaded" placeholder="Unloaded" >
        </div>
        <div><label type="number" id="result" ></label></div>
        <button type="submit" id="button" onclick="calc()">Click To 
Convert</button>
</body>
</html>

\$\endgroup\$
3
  • 1
    \$\begingroup\$ "total lbs * 0.0005 = weight in tons" That's quite a rounding error you produce there, almost 10% just with one action. Are you sure you want to do this? \$\endgroup\$
    – Mast
    Jul 2 '18 at 21:21
  • 1
    \$\begingroup\$ @Mast There are many kinds of tons, some of which are 2000 pounds. \$\endgroup\$ Jul 3 '18 at 19:17
  • \$\begingroup\$ @200_success Interesting, you're right. Our ton is your metric ton. Other ton(ne)s are still in use it would seem. \$\endgroup\$
    – Mast
    Jul 3 '18 at 20:01
1
\$\begingroup\$

Please don't take any of these comments personally, it's clear you are learning and attempting to write clean code and we all started right where you are. With that said I have gone through your code and added comments to outline some things I would change or do differently.

function calc() {
    // You should do some basic checks to make sure this value is actually a number
    // i.e. 
    // parse the flaot
    let loadedAmount = parseFloat(document.getElementById("loaded").value);
    // check if it's nan (Not A Number)
    if (isNaN(loadedAmount)) { alert('Invalid load amount'); return; }

    let loadedAmount = document.getElementById("loaded").value;
    let unloadedAmount = document.getElementById("unloaded").value;
   
    // this line is doing to much and makes it hard to read and will make it hard to maintain later in life.
    // variables do have some overhead but not enough to warrant not using them to make your code cleaner.
    let final = document.getElementById("result").value = Math.round(`${(loadedAmount - unloadedAmount) * 0.0005}` * 100)/100; 
    let resultInPounds = document.getElementById("result").value = Math.round(`${(loadedAmount - unloadedAmount)}`);

    // try something along these lines, i have no idea if your calculations are correct 
    let final = Math.Round((((loadedAmount - unloadedAmount) * 0.0005) * 100)/100);
    let resultInPounds = Math.Round(loadedAmount - unloadedAmount);

    document.getElementById("result").value = final;
    document.getElementById("result").value = resultInPounds;
    // you can see this is much easier to follow and understand what it's intention is.

    // You don't need to do these checks here if you've moved them above this so you can remove all of this code. Your variables will already have numbers in them and you would have already presented and errors to the user
    if (!loadedAmount) {
       document.getElementById("loaded").value=alert("waiting for an initial value for loaded");
    } else {
        loadedAmount = parseInt(loadedAmount);
    }
    if (!unloadedAmount) {
        document.getElementById("unloaded").value = alert("waiting for an initial value for unloaded");
       
    } else {
        unloadedAmount = parseInt(unloadedAmount);
    }
    if (loadedAmount < unloadedAmount) {
        alert("loaded amount must be greater than unloaded amount");
    } 

    // this is fine
    console.log(`loadedAmount: ${loadedAmount}`);
    console.log(`unloadedAmount: ${unloadedAmount}`);
    // this is fine although you are using templates before this and suddenly stopped, maybe use templates here or change the others to not use template. 
    document.getElementById("result").innerHTML=final + " Tons or " + resultInPounds + " lbs";
    // the variable 'result' doesn't exist anywhere and will always log undefined 
    console.log(result);
    
}
<!DOCTYPE html>
<html>
<head> 
    <meta charset="utf-8">
<title></title>
<style>
   
</style>
</head>
<body>
    <header>Negaunee Iron and Metal</header>
    <script src="app.js"></script>
        <div >input loaded pounds
            <input type="number"  id="loaded" placeholder="Loaded...">
        </div>
        <div > input unloaded pounds
            <input type="number" id="unloaded" placeholder="Unloaded" >
        </div>
        <div><label type="number" id="result" ></label></div>
        <button type="submit" id="button" onclick="calc()">Click To 
Convert</button>
</body>
</html>

Based on some feedback here is an example that implements all the changes suggested so far.

window.calc = function() {
  var self = this;

  // You should do some basic checks to make sure this value is actually a number
  // i.e. 
  // parse the float
  self.loadedAmount = parseFloat(document.getElementById("loaded").value);
  self.unloadedAmount = parseFloat(document.getElementById("unloaded").value);
  self.percentage = 0.0005;

  if (isNaN(self.loadedAmount) || isNaN(self.unloadedAmount)) {
    // make this a more meaningful message
    alert('Invalid numbers');
    // end the function, no calculations can be done
    return;

  }

  // do your calculations
  self.final = Math.round((self.loadedAmount - self.unloadedAmount) * self.percentage * 100 / 100);
  self.resultInPounds = Math.round(self.loadedAmount - self.unloadedAmount);

  // display the results
  document.getElementById("result").value = `${self.final} Tons or ${self.resultInPounds} lbs`;

  // log the results
  console.log(`loadedAmount: ${self.loadedAmount}`);
  console.log(`unloadedAmount: ${self.unloadedAmount}`);
  // the variable 'result' doesn't exist anywhere and will always log undefined 
  console.log(`${self.final} Tons or ${self.resultInPounds} lbs`);

}
<h3>Negaunee Iron and Metal</h3>
<label for="loaded">Loaded pounds</label>
<input type="number" id="loaded">
<br>
<label for="unloaded">Unloaded pounds</label>
<input type="number" id="unloaded">
<br>
<button type="button" id="button" onclick="window.calc()">Convert</button>
<br>
<output for="loaded unloaded" id="result"></output>

\$\endgroup\$
2
  • \$\begingroup\$ in my opinion, you shouldn't keep his code and your correction in the same file. It could be confusing, e.g. resultInPounds and final are both defined twice. \$\endgroup\$
    – Ivan
    Jul 3 '18 at 18:13
  • \$\begingroup\$ Good point however I was more or less trying to highlight the suggestions. I'll make a revision only containing my suggested changes. \$\endgroup\$
    – Adam H
    Jul 3 '18 at 18:23
2
\$\begingroup\$

The label element can be used for the labels of the input fields.

The placeholder attribute shouldn’t be used for labeling.

The output element can be used for the result. (You are using a label, which is not appropriate; and note that label can’t have a type attribute.)

Maybe a button with the button type is more appropriate here. (Button type “button” vs. “submit”)

This results in:

<label for="loaded">Loaded pounds</label>
<input type="number" id="loaded">

<label for="unloaded">Unloaded pounds</label>
<input type="number" id="unloaded">

<button type="button" id="button" onclick="calc()">Convert</button>

<output for="loaded unloaded" id="result"></output>
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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