Problem
I have the following HTML + JavaScript code:
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css" integrity="sha384-JcKb8q3iqJ61gNV9KGb8thSsNjpSL0n8PARn9HuZOnIxN0hoP+VmmDGMN5t9UJ0Z" crossorigin="anonymous">
<div class="container mt-4">
<!-- action="/reports_send/21-TEMP-01a" method="POST" -->
<h3>Form:</h3>
<form id="form" class="mt-4 mb-4" >
<div style="border: 1px solid black; padding: 40px; border-radius: 25px;">
<div class="container mt-4">
<div id="errors" class="mt-4"></div>
</div>
<h4>Select Room</h4>
<div id="RoomSelect">
<select id="RoomMenu" class="form-control mb-4">
<!-- Drying Room 1 -->
<option value="dry-1">Drying Room 1</option>
<!-- Drying Room 2 -->
<option value="dry-2">Drying Room 2</option>
<!-- Dry Store-->
<option value="dry-3">Dry Store</option>
</select>
</div>
<div id="RoomInputs">
<!-- Drying Room 1 -->
<div class="form-group" id="dry-1">
<!-- Title -->
<h4>Drying Room 1</h4>
<!-- All temperatures -->
<div class="temperatures">
<!-- Actual Temperature -->
<label>Temperature °C - <strong>Actual</strong></label>
<input type="number" class="form-control" name="actual-temp-1">
<!-- Minimum Temperature -->
<label>Temperature °C - <strong>Minimum</strong></label>
<input type="number" class="form-control" name="min-temp-1">
<!-- Maximum Temperature -->
<label>Temperature °C - <strong>Maximum</strong></label>
<input type="number" class="form-control" name="max-temp-1">
</div>
<br>
<br>
<!-- All humidity -->
<div class="humidity">
<!-- Actual Humidity -->
<label>Relative Humidity - <strong>Actual</strong></label>
<input type="number" class="form-control" name="actual-temp-2">
<!-- Minimum Humidity -->
<label>Relative Humidity - <strong>Minimum</strong></label>
<input type="number" class="form-control" name="min-temp-2">
<!-- Maximum Humidity -->
<label>Relative Humidity - <strong>Maximum</strong></label>
<input type="number" class="form-control" name="max-temp-2">
</div>
</div>
<!-- Drying Room 2 -->
<div class="form-group" id="dry-2">
<!-- Title -->
<h4>Drying Room 2</h4>
<!-- All temperatures -->
<div class="temperatures">
<!-- Actual Temperature -->
<label>Temperature °C - <strong>Actual</strong></label>
<input type="number" class="form-control" name="actual-temp-3">
<!-- Minimum Temperature -->
<label>Temperature °C - <strong>Minimum</strong></label>
<input type="number" class="form-control" name="min-temp-3">
<!-- Maximum Temperature -->
<label>Temperature °C - <strong>Maximum</strong></label>
<input type="number" class="form-control" name="max-temp-3">
</div>
<br>
<br>
<!-- All humidity -->
<div class="humidity">
<!-- Actual Humidity -->
<label>Relative Humidity - <strong>Actual</strong></label>
<input type="number" class="form-control">
<!-- Minimum Humidity -->
<label>Relative Humidity - <strong>Minimum</strong></label>
<input type="number" class="form-control">
<!-- Maximum Humidity -->
<label>Relative Humidity - <strong>Maximum</strong></label>
<input type="number" class="form-control">
</div>
</div>
<!-- Dry Store -->
<div class="form-group" id="dry-3">
<!-- Title -->
<h4>Dry Store</h4>
<!-- All temperatures -->
<div class="temperatures">
<!-- Actual Temperature -->
<label>Temperature °C - <strong>Actual</strong></label>
<input type="number" class="form-control">
<!-- Minimum Temperature -->
<label>Temperature °C - <strong>Minimum</strong></label>
<input type="number" class="form-control">
<!-- Maximum Temperature -->
<label>Temperature °C - <strong>Maximum</strong></label>
<input type="number" class="form-control">
</div>
<br>
<br>
<!-- All humidity -->
<div class="humidity">
<!-- Actual Humidity -->
<label>Relative Humidity - <strong>Actual</strong></label>
<input type="number" class="form-control">
<!-- Minimum Humidity -->
<label>Relative Humidity - <strong>Minimum</strong></label>
<input type="number" class="form-control">
<!-- Maximum Humidity -->
<label>Relative Humidity - <strong>Maximum</strong></label>
<input type="number" class="form-control">
</div>
</div>
</div>
</div>
</form>
</div>
<button id="submit-btn" class="btn btn-primary">Submit</button>
<script>
// UI CONTROLLER
var UIController = (() => {
// Store DOM Strings
var DOMStrings = {
room_options: '#RoomMenu',
btn: '#submit-btn'
};
return {
// Hide all Div Elements
hideDivElements: () => {
console.log(document.querySelector(DOMStrings.room_options).value);
for(const option of document.querySelector(DOMStrings.room_options).options) {
document.querySelector(`#${option.value}`).style.display = "none";
}
if(document.querySelector(DOMStrings.room_options).value === 'dry-1') {
document.querySelector('#dry-1').style.display = "block";
} else if (document.querySelector(DOMStrings.room_options).value === 'dry-2') {
document.querySelector('#dry-2').style.display = "block";
} else if (document.querySelector(DOMStrings.room_options).value === 'dry-3') {
document.querySelector('#dry-3').style.display = "block";
}
},
// Return DOM Strings
getDOMStrings: () => {
return DOMStrings;
}
}
})();
// GLOBAL APP CONTROLLER
var controller = ((UICtrl) => {
var setupEvenetListeners = () => {
// Get dom strings from UI Controller
var DOM = UICtrl.getDOMStrings();
// On change
document.querySelector(DOM.room_options).addEventListener('change', () => {
UICtrl.hideDivElements();
});
}
return {
init: () => {
console.log("Application has started");
UICtrl.hideDivElements(); // Hide elements at start of program
setupEvenetListeners();
}
}
})(UIController);
controller.init();
</script>Question
In my if statement – I am showing specific elements based on selected option. I was wondering, If there is a way not to hard code dry-1 dry-2 dry-3 within my if statement and have an alternative way of doing it?
Also – I wouldn’t mind to hear some reviews on my current code 🙂 I have decided to start getting used to using controllers etc..
Solution
Options Rather than repeatedly selecting the room_options value, put it into a variable, then reference that variable:
const { value } = document.querySelector(DOMStrings.room_options);
// reference value here
// instead of referencing document.querySelector(DOMStrings.room_options).value
But, since the option values exactly match the IDs you want to show, simply pass the option value into a single querySelector, no looping needed:
const { value } = document.querySelector(DOMStrings.room_options);
document.querySelector(`#${value}`).style.display = "block";
Comments You have
<!-- All humidity -->
<div class="humidity">
<!-- Actual Humidity -->
<label>Relative Humidity - <strong>Actual</strong></label>
<input type="number" class="form-control">
<!-- Minimum Humidity -->
<label>Relative Humidity - <strong>Minimum</strong></label>
<input type="number" class="form-control">
<!-- Maximum Humidity -->
<label>Relative Humidity - <strong>Maximum</strong></label>
<input type="number" class="form-control">
</div>
These sorts of comments don’t tell the reader anything they wouldn’t know already by looking at the next line. Best to leave the comments out entirely. Use comments to explain why you’re doing something, or what the purpose of a variable or section is if it’s not already obvious.
Variable names
hideDivElementshides elements, but it also shows the one active element. Maybe call itshowActiveElementinstead?setupEvenetListenersis misspelledvar DOM = UICtrl.getDOMStrings();– but the return value is a plain object, not a/the DOM. For consistency, maybe call it the same name everywhere,DOMStrings.- The property
room_optionsholds the selector string#RoomMenu, which points to a single<select>tag, not a collection of options. Maybe call the propertyroomSelectIdinstead? (Probably good to follow the usual conventions and usecamelCasein most cases in JS)
DOMStrings and encapsulation I’m a bit skeptical of the DOMStrings object in its current form – an object which just contains strings that can be used to select elements doesn’t feel all that beneficial to me. You might consider selecting the elements themselves, and exporting that instead of selector strings:
const elements = {
roomSelect: document.querySelector('#RoomMenu'),
// below: be precise, `submitBtn` is easier to understand than just `btn`
submitBtn: document.querySelector('#submit-btn'),
};
Still, attempting encapsulation and abstraction like this is a bit weird on the front-end when all elements are essentially global. The HTML is intrinsicly coupled with your JS (and usually, your CSS as well). If you change something in one place, you’ll have to change it everywhere. It looks like your DOMStrings might be an attempt to slightly mitigate this, so as to allow you to just change a DOMString when something needs to be changed, rather than search-and-replace throughout your application.
It’d be a big change, but if that’s the sort of thing you’re concerned about, for a larger application, you could consider experimenting with a framework like React instead, allowing for better encapsulation and functional composition of components. For example, this application could be rendered with something like the following:
<Rooms roomNames={['Drying Room 1', 'Drying Room 2', 'Dry Store']} />
leading to a Room component containing the labels and inputs, without duplicating new HTML for each section, and without having to specify magic strings connecting the DOM to event listeners. It’s just something to consider, once you’re comfortable with JS.
Modern syntax If you’re going to use ES2015+ syntax (which you are, and you should), best to use it everywhere – declare variables with const whenever possible (and never with var). (But, if you want the code to be compatible with ancient obsolete browsers like IE11, transpile it to ES5 using Babel for production)