Cardinal direction enum from range input

Question

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>

\$\begingroup\$ @konijn I did not see this, caching issue perhaps? \$\endgroup\$

dfhwze
– dfhwze

2019-08-26 11:44:40 +00:00
Commented Aug 26, 2019 at 11:44 — dfhwze
– dfhwze, Commented Aug 26, 2019 at 11:44

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2019-08-27 15:25:16Z

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" ¹ (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);

Thank you, this was very helpful. I have taken your input and updated my CodePen: codepen.io/bbbenji/pen/JjPGNmY — bskool
– bskool, Commented Aug 28, 2019 at 8:50

Nathan Walther · Accepted Answer · 2019-08-27 00:48:42Z

2

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

answered Aug 27, 2019 at 0:48

Nathan Walther

2161 silver badge4 bronze badges

Add a comment |

Stack Exchange Network

Cardinal direction enum from range input

2 Answers 2

Evaluation

Suggestions

Semicolon terminators

Selecting elements by class vs id attribute

Global variables

Excess closures

You must log in to answer this question.

Hot Network Questions

Cardinal direction enum from range input

2 Answers 2

Evaluation

Suggestions

Semicolon terminators

Selecting elements by class vs id attribute

Global variables

Excess closures

You must log in to answer this question.

Related

Hot Network Questions