1
\$\begingroup\$

I use this function to to do some calculations based on 4 select inputs and one number input, I don't have experience in Js, I did some research in w3schools then I ended up with the function below. The Js function is working exactly as I want, but I feel it is not the perfect syntax and it could be shorter and somehow cleaner, any advice would be appreciated.

function calculate() {
  var x = parseFloat(document.getElementById("Lang_from").value);
  var y = parseFloat(document.getElementById("Lang_to").value);
  var q = parseFloat(document.getElementById("quantity").value);
  var s = parseFloat(document.getElementById("subject").value);
  var f = parseFloat(document.getElementById("file_type").value);
  var xx = document.getElementById('Lang_from').selectedOptions[0].text;
  var yy = document.getElementById('Lang_to').selectedOptions[0].text;
  var ff = document.getElementById('file_type').selectedOptions[0].text;
  var ss = document.getElementById('subject').selectedOptions[0].text;

  document.getElementById("total_price").innerHTML = (x + y) * q + (s + f);
}
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css" integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">
<html>

<body>

  <div class="pricing_row">
    <div class="pricing_column">
      <div class="form-group">
        <h4 class="pricing_lable">From</h4>
        <select id="Lang_from" name="Lang_from" value="" class="form-control pricing_input">
          <option id="en2ar" value="0.025">English</option>
          <option>German</option>
        </select>
      </div>
      <div class="form-group">
        <h4 class="pricing_lable">To</h4>
        <select id="Lang_to" name="Lang_to" value="" class="form-control pricing_input">
          <option value="0.025">German</option>
          <option>English</option>
        </select>
      </div>
    </div>
    <div class="pricing_column">
      <div class="form-group">
        <h4 class="pricing_lable">File type</h4>
        <select id="file_type" name="file_type" value="" class="form-control pricing_input">
          <option value="0">TXT</option>
          <option value="3">MS word</option>
          <option value="5">PDF (+5$)</option>
          <option value="10">Hand Writing</option>
        </select>
      </div>
      <div class="form-group">
        <h4 class="pricing_lable">Subject</h4>
        <select id="subject" name="subject" value="" class="form-control pricing_input">
          <option value="0">General</option>
          <option value="10">Technical / IT</option>
          <option value="15">Medical</option>
          <option value="5">Press</option>
        </select>
      </div>
    </div>
    <div class="pricing_column">
      <div class="form-group">
        <h4 class="pricing_lable">Word count</h4>
        <input type="number" id="quantity" name="quantity" min="500" value="1000" class="form-control pricing_input pricing_input_number">
      </div>
      <div class="form-group">
        <button id="calculate" type="button" class="btn btn-primary pricing_btn_calc" onclick="calculate()">Calculate</button>

        <div id="total_price"></div>
      </div>
    </div>
  </div>


</body>

</html>

\$\endgroup\$
1
  • \$\begingroup\$ Just a quick tip that using destructuring assignment with the HTMLFormElement API can shorten code (i.e. const [from, to, fileType, subject, quantity] = Array.from(form.elements).map(el => Number(el.value)) where form is a reference to a <form> element, which would be the appropriate semantic element for user input controls. \$\endgroup\$
    – morbusg
    Aug 20, 2019 at 16:42

1 Answer 1

3
\$\begingroup\$

Access elements via ID

You can get access to elements directly via their ID, if you ensure that each elements id and or name is unique withing the page and JavaScripts global scope.

var x = parseFloat(document.getElementById("Lang_from").value);

becomes

var x = parseFloat(Lang_from.value);

In the example at the bottom I have removed all the names from the HTML. You have not indicated that this is a submit-able form and thus you can do without the extra markup.

Note that forms do provide additional information for clients with special needs. For content that faces the world (public) always consider Accessibility. Dealing with font end interaction (not using forms) you should be familiar with WAI-ARIA and how it makes content meaningful and accessible to everyone .

Use Number rather than parseFloat

Rather than use parseFloat use Number to convert a string number to a Number type

var x = parseFloat(Lang_from.value);

becomes

var x = Number(Lang_from.value);

or you can coerce a string representing a number using an operator. Commonly + is used

var x = + Lang_from.value;

JavaScripts has automatic type coercion and will force the string to a number, Note that + will not work as var x = "10" + Lang_from.value; as the type is set by "10" and + can operate on strings (concats)

Define appropriate variable types

The variables you use are never modified. JS has several variable types. var, let, and const. Get in a habit of using the most appropriate type, for variables that do not change or should not change use const

const x = +Lang_from.value;

Naming is an important programming skill to master

Always pay special care to the names you use.

Some of your variable names have absolutely no relationship to their content (eg x, y, and maybe f)

Using abbreviations is OK and can reduce clutter, using the first letter of each word in the ID is OK (BUT DEFINING NAME and USING NAME must be no more than a page of apart, as the need to hunt for meaning is a distraction that aids bugs getting in)

const lf = +Lang_from.value;
const lt = +Lang_to.value;
const q =  +quantity.value;
const s =  +subject.value;
const ft = +file_type.value;

Avoid intermediates if possible

Rather than the many names, calculate the total as you access the data.

var total = (+Lang_from.value) + (+Lang_to.value); // The extra + to coerce to number
total *= quantity.value;  // *= same as total = total * quantity.value;
                          // The extra + not ended if the operation on the value is
                          // not +
total += +subject.value;
total += +file_type.value;

NO! don't do this... <button onclick="functionName()">

Avoid putting code within the markup. The event listener for calculate should be added via JavaScript when the page has loaded. Use "load" or "DOMContentLoaded" events

addEventListener("load", function() {  // waits for the page to load
     calculate.addEventListener("click", calculateTotal);
});

// or using arrow function and a slightly earlier event that fires when 
// all the HTML content has loaded and been parsed (image and the like may not be ready)
addEventListener("DOMContentLoaded", () => {  // waits for the page to load
     calculate.addEventListener("click", calculateTotal);
});


// NOTE that the element id is calculate which conflicted with the JavaScript function 
// name calculate. I renamed the function to calculateTotal ( which I would have done
// even if there was no name conflict)

Loose ends are bad practice

Keep the code clean and do not include code that has no purpose. You have 5 variables that get the selections yet you don't use them. They should not be in the function. If you put them their because you intend to use them at another time, finish the code or add a comment with that code eg /* Todo: Show selection names */ else the code is considered incomplete (easy to forget or worse)

Use appropriate properties

When adding content to an element and it is just text (no HTML content) use the node property Node.textContent to set the text. (Elements inherit properties from Node so can be used on most elements)

Example

Basic Markup only

I have removed all irrelevant information from the Markup, (forms, styling wrappers, name/id duplicates, property duplicates, class name)

I removed the references to the CSS styles. Looking good is important, but start at the core and work up, don't start with looking good, that is why we have CSS, to seperate the two.

"use strict";
addEventListener("DOMContentLoaded", () => {
  calculate.addEventListener("click", calculateTotal);
});

function calculateTotal() {
  var total = (+Lang_from.value) + (+Lang_to.value); 
  total *= quantity.value; 
  total += (+subject.value) + (+file_type.value);
  total_price.textContent = "$" + total.toFixed(2);
}
<h4>From</h4>
<select id="Lang_from">
  <option value="0.025">English</option>
  <option>German</option>
</select>
<h4>To</h4>
<select id="Lang_to">
  <option value="0.025">German</option>
  <option>English</option>
</select>
<h4>File type</h4>
<select id="file_type">
  <option value="0">TXT</option>
  <option value="3">MS word</option>
  <option value="5">PDF (+5$)</option>
  <option value="10">Hand Writing</option>
</select>
<h4>Subject</h4>
<select id="subject">
  <option value="0">General</option>
  <option value="10">Technical / IT</option>
  <option value="15">Medical</option>
  <option value="5">Press</option>
</select>
<h4>Word count</h4>
<input type="number" id="quantity" min="500" value="1000">
<button id="calculate">Calculate</button>
<h4>Total price</h4>
<div id="total_price"></div>

Beginners note

You will notice the string "use strict"; at the top of the JavaScript, it is a directive that forces JavaScript to run in strict mode.

The name may suggest this is something for experienced coders, quite the contrary as experienced JS coders seldom make the errors this will trap.

Experienced JS coders always use strict mode because its faster and because everyone make mistakes.

Ergo: Always run your javascript in strict mode using the directive "use strict";

This example has no semantic meaning to some

I have not used ARIA to add semantic meaning to the content as I do not think its applicable in this case and would make for a way too long already too long answer.

Slight change to result

I noted that the quantity input is unconstrained in its precision (Allows fractions). When you output values, especially monetary values, be careful to include the correct rounding and precision. A money value ideally always starts with the currency type (I used $ and the value is written with full precision Dollars.Cents eg $10.00.

JavaScripts Number (floating point double) is only an approximation of numbers, in a few calculation a number can easily gain a rounding error and just display the value raw will end you with displayed values like 10.00000000000003 or 3e-17 both not good things to show when it involves money,

References

The site you used as a reference has a bad wrap, being incomplete and out of date, Though it has been years since I have had a good look, so if this remains true I am unsure.

I better reference site is MDN JavaScript and the main landing MDN it has a mostly complete reference of Web technologies, plenty of learning resources (tutorials, examples and the like), and if you need to get to the definitive resource they do provide links to the all the standards documentation (Very dry and hard to read resource of everything) that is applicable.

\$\endgroup\$

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.