2
\$\begingroup\$

Still learning JS and I made the following function to convert degrees into cardinal directions respectively. My first iteration of the code was over 50 lines and I was able to get it down to 13 using a for loop.

Is there a way to optimize or simplify the code down even more? Is my logic and implementation ideal?

https://codepen.io/bbbenji/pen/JjPGNmY

function setCardinalDirection() {
  value = document.querySelector('#orientation').value // Get current value from range input
  document.querySelector(".degrees").textContent = value // Inject current input value into label
  direction = document.querySelector(".direction") // Define intercardinal direction display element

  degrees = 22.5 // Define range between two intercardinal directions
  cardinal = ["N", "NNE", "NE", "ENE", "E", "ESE", "SE", "SSE", "S", "SSW", "SW", "WSW", "W", "WNW", "NW", "NNW", "N"]

  for ( i = cardinal.length; i >= 0; i-- ) { // Iterate through cardinal array backwards
    if ( value < degrees/2 + degrees * i ) {
      cardinalOut = cardinal[i]
    }
  }

  direction.textContent = cardinalOut // Inject current cardinal value into label
}

document.querySelector("#orientation").addEventListener('input', function(event) {
 setCardinalDirection()
})

window.addEventListener("load", function(){
 setCardinalDirection()
})
<form>
  <label for ="orientation">
    Orientation: <strong><span class="degrees"></span>° (<span class="direction"></span>)</strong>
  </label>
  <br />
  <input id="orientation" type="range" min="0" max="360" step="1" value="145">
</form>

\$\endgroup\$
  • 1
    \$\begingroup\$ @konijn I did not see this, caching issue perhaps? \$\endgroup\$ – dfhwze Aug 26 at 11:44
3
\$\begingroup\$

Evaluation

For somebody learning Javascript this code isn't bad, however it does have some inefficiencies and practices that are frowned upon (e.g. Global variables). See the suggestions below for advice about improving it.

Suggestions

Semicolon terminators

Unless you are intimately familiar with the statements that need to be terminated by a semicolon it is wise to always terminate each line with a semi-colon.

Selecting elements by class vs id attribute

The first line selects an element from the DOM by the id attribute - i.e. document.querySelector('#orientation'). The next line uses a class selector to select an element: document.querySelector(".degrees"). If there is only one element with class name degrees that matters for this application then an id attribute would be more appropriate than class.

Also, it isn't wrong to use document.querySelector() to get elements by id but using document.getElementById() "is definitely faster" 1 (see this jsPerf test for comparison). Similarly document.getElementsByClassName() is also faster. I would suggest only using document.querySelector() when there is a complex selector (e.g. form lable[for="direction']

Global variables

Any variable not declared with a keyword like var, const or let is considered global. For a small application like this it likely wouldn't lead to any issues but in a larger application it could lead to unintentional side-effects if the same name is used in different functions.

Excess closures

The lines to add event listeners can be simplified:

document.querySelector("#orientation").addEventListener('input', function(event) {
  setCardinalDirection()
})

window.addEventListener("load", function(){
 setCardinalDirection()
})

Instead of wrapping the calls to setCardinalDirection() in an extra function, just pass the name of the function:

document.querySelector("#orientation").addEventListener('input', setCardinalDirection); 
window.addEventListener("load", setCardinalDirection);
\$\endgroup\$
2
\$\begingroup\$

You can replace the for loop with cardinalOut = cardinal[Math.floor((value / degrees) + 0.5)].

\$\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.