3
\$\begingroup\$

Goal

I have developed a random check that I have thought of. I have used purely JavaScript to get used to the language and learn it. I have also used JavaScript to create elements, classes, id etc... and at the end, I have a simple validation check to check if all inputs have been filled.

I would to soon improve the validation check by prompting to show which field is empty.

Finally, I'd like to only know where I can improve my code, in terms of simplicity, how I can make it simpler? I'll be happy to hear any recommendations!

Code:

    <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.5.1/jquery.min.js"></script>
    <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">
    <h3>Form:</h3>
    <form id="form" class="mt-4 mb-4" action="/reports_send/21-TEMP-01a" method="POST">

        <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">

            </div>
            
            <div id="RoomInputs">

            </div>

            <button class="btn btn-primary">Submit</button>
        </div>


    </form>

</div>

 <script src="https://code.jquery.com/jquery-3.5.1.slim.min.js" integrity="sha384-DfXdz2htPH0lsSSs5nCTpuj/zy4C+OGpamoFVy38MVBnE+IbbVYUew+OrCXaRkfj" crossorigin="anonymous"></script>
    <script src="https://cdn.jsdelivr.net/npm/popper.js@1.16.1/dist/umd/popper.min.js" integrity="sha384-9/reFTGAW83EW2RDu2S0VKaIzap3H66lZH81PoYlFhbGU+6BZp6G7niu735Sk7lN" crossorigin="anonymous"></script>
    <script src="https://stackpath.bootstrapcdn.com/bootstrap/4.5.2/js/bootstrap.min.js" integrity="sha384-B4gt1jrGC7Jh4AgTPSdUtOBvfO8shuf57BaghqFfPlYxofvL8/KUEfYiJOMMV+rV" crossorigin="anonymous"></script>


<script>
// -----------------------------PART-1---------------------------------------------------

// Will be used to add select element
var RoomSelectId = document.getElementById("RoomSelect");

// Create select element
var selectElement = document.createElement("select");
selectElement.setAttribute("id", "RoomMenu");
selectElement.setAttribute("class", "form-control mb-4");

// Drying room 1
var dryingRoom1 = document.createElement("option");
dryingRoom1.value = "DryingRoom1";
dryingRoom1.text = "Drying Room 1";
selectElement.appendChild(dryingRoom1);

// Drying room 2
var dryingRoom2 = document.createElement("option");
dryingRoom2.value = "DryingRoom2";
dryingRoom2.text = "Drying Room 2";
selectElement.appendChild(dryingRoom2);

// Dry Store
var dryStore = document.createElement("option");
dryStore.value = "DryStore";
dryStore.text = "Dry Store";
selectElement.appendChild(dryStore);


RoomSelectId.appendChild(selectElement);


// -----------------------------PART-1-END-----------------------------------------------

// -----------------------------PART-2---------------------------------------------------

// Creating inputs for temperature and humidity

// Get div of room inputs
var roomInputsId = document.getElementById("RoomInputs");

// Get all options
var roomOptions = document.getElementById("RoomMenu");

// Create all inputs, such as temperature and humidity
for(var i = 0; i < roomOptions.length ; i++) {
    var divElement = document.createElement("div");
    divElement.setAttribute("class", `form-group RoomDivEl ${roomOptions.options[i].value}`);
    divElement.style.display = "none";

    //Title
    var title = document.createElement("h4");
    title.appendChild(document.createTextNode(roomOptions.options[i].innerHTML));
    divElement.appendChild(title);

    // Temperature

    // Actual
    var actualTemp = document.createElement("label");
    actualTemp.innerHTML = "Temperature °C - <strong>Actual</strong>";
    divElement.appendChild(actualTemp);

    var actualTempInput = document.createElement("input");
    actualTempInput.setAttribute("class", "form-control");
    actualTempInput.setAttribute("type", "number");
    actualTempInput.setAttribute("name", `ActualTemp${roomOptions.options[i].value}`);
    divElement.appendChild(actualTempInput);


    // Minimum
    var minTemp = document.createElement("label");
    minTemp.innerHTML = "Temperature °C - <strong>Minimum</strong>";
    divElement.appendChild(minTemp);

    var minTempInput = document.createElement("input");
    minTempInput.setAttribute("class", "form-control");
    minTempInput.setAttribute("type", "number");
    minTempInput.setAttribute("name", `minTemp${roomOptions.options[i].value}`);
    divElement.appendChild(minTempInput);

    // Maximum
    var maxTemp = document.createElement("label");
    maxTemp.innerHTML = "Temperature °C - <strong>Maximum</strong>";
    divElement.appendChild(maxTemp);

    var maxTempInput = document.createElement("input");
    maxTempInput.setAttribute("class", "form-control");
    maxTempInput.setAttribute("type", "number");
    maxTempInput.setAttribute("name", `maxTemp${roomOptions.options[i].value}`);
    divElement.appendChild(maxTempInput);


    // Actual
    var actualHumidity = document.createElement("label");
    actualHumidity.innerHTML = "Relative Humidity - <strong>Actual</strong>";
    divElement.appendChild(actualHumidity);

    var actualHumidityInput = document.createElement("input");
    actualHumidityInput.setAttribute("class", "form-control");
    actualHumidityInput.setAttribute("type", "number");
    actualHumidityInput.setAttribute("name", `actualHumidity${roomOptions.options[i].value}`);
    divElement.appendChild(actualHumidityInput);


    // Invisible input box to be used to get Room Name
    var invisibleRoomName = document.createElement("input");
    invisibleRoomName.setAttribute("name", `RoomName${roomOptions.options[i].value}`);
    invisibleRoomName.setAttribute("value", `${roomOptions.options[i].innerHTML}`);
    invisibleRoomName.style.display = "none";
    divElement.appendChild(invisibleRoomName);

    // Minimum
    var minHumidity = document.createElement("label");
    minHumidity.innerHTML = "Relative Humidity - <strong>Minimum</strong>";
    divElement.appendChild(minHumidity);

    var minHumidityInput = document.createElement("input");
    minHumidityInput.setAttribute("class", "form-control");
    minHumidityInput.setAttribute("type", "number");
    minHumidityInput.setAttribute("name", `minHumidity${roomOptions.options[i].value}`);
    divElement.appendChild(minHumidityInput);


    // Maximum
    var maxHumidity = document.createElement("label");
    maxHumidity.innerHTML = "Relative Humidity - <strong>Maximum</strong>";
    divElement.appendChild(maxHumidity);

    var maxHumidityInput = document.createElement("input");
    maxHumidityInput.setAttribute("class", "form-control");
    maxHumidityInput.setAttribute("type", "number");
    maxHumidityInput.setAttribute("name", `maxHumidity${roomOptions.options[i].value}`);
    divElement.appendChild(maxHumidityInput);

    // Combine all into the div element
    roomInputsId.appendChild(divElement);
}

// Set desfault option to index of 0
for(var i = 0; i < roomInputsId.getElementsByClassName("RoomDivEl").length; i++) {
    roomInputsId.getElementsByClassName("RoomDivEl")[0].style.display = "block";
}

// -----------------------------PART-2-END-----------------------------------------------

// -----------------------------PART-3---------------------------------------------------

// Event listener to access its chuld class and target selected option
roomOptions.addEventListener("change", (event) => {
    const selectOption = roomInputsId.getElementsByClassName(event.target.value);
    
    // Hide all Divs
    for(var i = 0; i < roomInputsId.getElementsByClassName("RoomDivEl").length; i++) {
        roomInputsId.getElementsByClassName("RoomDivEl")[i].style.display = "none";
    }

    // Show selected div
   selectOption[0].style.display = "block";
})

// -----------------------------PART-3-END-----------------------------------------------


// -----------------------------PART-4---------------------------------------------------


// Check if every temp and humidity hass ben done

document.getElementById("form").addEventListener("submit", (e) => {

    // Error messages array used in the loop below
    var errorMessages = [];

    // For loop to go over each Fridge and Freezer temperature value
    for(var i = 0; i < document.forms["form"].getElementsByTagName("input").length; i++) {
       
        // Checking if any values is empty
        if(!document.forms["form"].getElementsByTagName("input")[i].value) {
            errorMessages.push("Fill");
        }

    }
    if(errorMessages.length > 0) {
        e.preventDefault();
        document.getElementById("errors").innerHTML = '<div class="alert alert-danger" role="alert"><p><strong>Please complete all the Temperatures and Humidity Checks</strong></p></div>';
    }
});

// -----------------------------PART-4-END-----------------------------------------------



</script>

\$\endgroup\$
5
\$\begingroup\$

setAttribute? When assigning properties to elements, I'd prefer to use dot notation assignment instead of setAttribute - it's more concise and a bit easier to read and write. For example:

selectElement.setAttribute("id", "RoomMenu");
selectElement.setAttribute("class", "form-control mb-4");

can turn into

selectElement.id = 'RoomMenu';
selectElement.className = 'form-control mb-4';

Use modern syntax It's 2020. For clean, readable code in a reasonably professional project, I'd recommend writing in the latest and greatest version of the language - or at least in ES2015. Modern syntax offers quite a few benefits, such as the ability to use const, concise arrow functions, and much more. If you're worried about browser compatibility, use Babel to transpile your code automatically into ES5 for production, while keeping the source code modern, readable, and concise.

You're already using template literals (which are ES2015) - might as well go the rest of the way.

Element creation DRYing You create a lot of elements dynamically, and then assign various properties and attributes. You could do this more elegantly by abstracting it into a function, and then calling that function whenever you need to make an element. For example, for Part 1, you could do:

const createElement = (tagName, parent, properties) => {
  const element = parent.appendChild(document.createElement(tagName));
  Object.assign(element, properties);
  return element;
};

// Will be used to add select element
const RoomSelectId = document.getElementById("RoomSelect");
const selectElement = createElement(
  'select',
  RoomSelectId,
  { id: 'RoomMenu', className: 'form-control mb-4' }
);
createElement(
  'option',
  selectElement,
  { value: 'DryingRoom1', text: 'Drying Room 1' }
);
createElement(
  'option',
  selectElement,
  { value: 'DryingRoom2', text: 'Drying Room 2' }
);
createElement(
  'option',
  selectElement,
  { value: 'DryStore', text: 'Dry Store' }
);

And so on.

Don't re-select elements you already have If you have a reference to an element already, eg:

var selectElement = document.createElement("select");
selectElement.setAttribute("id", "RoomMenu");

Then there's no need to select it again later with

var roomOptions = document.getElementById("RoomMenu");

That adds extra computation for no reason, and is confusing. Just keep using the old variable name of selectElement.

Manual iteration? Having to mess with indicies of an array manually is a bit ugly. When you have a collection you want to iterate over, and you don't care about the indicies, don't iterate over the indicies if possible - instead, just iterate over the collection. For example, in Part 2, instead of

for(var i = 0; i < roomOptions.length ; i++) {
  // numerous references to roomOptions.options[i]

you can use

for(const option of roomOptions.options) {
  // numerous references to option

Text insertion You do:

title.appendChild(document.createTextNode(roomOptions.options[i].innerHTML));

There are 2 issues here:

  • When you start with an empty element and want to populate it with text, it's easier to assign to its textContent than to go through document.createTextNode and appendChild
  • Unless you're deliberately setting or retrieving HTML markup from an element, it's more appropriate to use textContent than innerHTML. The code above can be replaced by:
title.textContent = option.textContent;

This applies to other areas of the code as well. You have many instances of .innerHTML = when you're only assigning text, so you should assign to the .textContent instead. (Using innerHTML is not only less appropriate and potentially slower, but it can result in arbitrary code execution when the HTML being set isn't trustworthy)

Selectors and array methods are great You have:

var errorMessages = [];
// For loop to go over each Fridge and Freezer temperature value
for(var i = 0; i < document.forms["form"].getElementsByTagName("input").length; i++) {
    // Checking if any values is empty
    if(!document.forms["form"].getElementsByTagName("input")[i].value) {
        errorMessages.push("Fill");
    }
}
if(errorMessages.length > 0) {
    e.preventDefault();
    document.getElementById("errors").innerHTML = '<div class="alert alert-danger" role="alert"><p><strong>Please complete all the Temperatures and Humidity Checks</strong></p></div>';
}

Rather than document.forms['form'].getElementsByTagName("input"), you can use a selector string to select the inputs which are children of #form:

document.querySelectorAll('#form input');

Selector strings are terse, flexible, and correspond directly to CSS selectors, and so are probably the preferred method of selecting elements. (You can change to this method in other places in the code as well, such as when you define selectOption)

Since you want to check if any of the inputs have an empty value, rather than pushing unexamined values to an array, use Array.prototype.some instead:

if ([...document.querySelectorAll('#form input')].some(input => !input.value)) {
    e.preventDefault();
    document.getElementById("errors").innerHTML = '<div class="alert alert-danger" role="alert"><p><strong>Please complete all the Temperatures and Humidity Checks</strong></p></div>';
}

Variable names You have:

  • roomInputsId: This is an element, not an ID; better to remove the "Id" suffix. But since it's an element, not multiple elements: the s makes it sound plural when it's not. Maybe call it roomInputsContainer instead?
  • RoomSelectId: Similar to above, but it's also using PascalCase. Ordinary variables in JS nearly always use camelCase - reserve PascalCase for classes, constructors, and namespaces, for the most part.
  • roomOptions: Like the first - this is a single element, so it shouldn't be plural

Overall If this is for something professional that needs to be maintained, and you have to do this sort of thing frequently on pages (dynamically creating, appending, removing, validating elements), I'd consider using a standard framework instead; they're a bit more maintainable in the long run over multiple developers and multiple years.

\$\endgroup\$
9
  • \$\begingroup\$ Appreciate your great responses! \$\endgroup\$ Sep 18 '20 at 21:01
  • \$\begingroup\$ Are you able to explain please what is happening in the if([...document.querySelectorAll... - I have never seen such thing before, especially the 3 dots .... Also - what would be way to point out/ highlight which field is missing a value? \$\endgroup\$ Sep 22 '20 at 12:37
  • \$\begingroup\$ I am thinking something like this: // Check empty for(var i = 0; i < document.querySelectorAll('#form input').length; i++) { e.preventDefault(); if(document.querySelectorAll('#form input')[i].value === "") { document.querySelectorAll('#form input')[i].style.backgroundColor = 'red'; } else { document.querySelectorAll('#form input')[i].style.backgroundColor = '#fff'; } } \$\endgroup\$ Sep 22 '20 at 12:45
  • \$\begingroup\$ That [...qSA()] turns what qSA returns into an array. It's called spread syntax developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… You could also iterate manually or use Array.from or Array.prototype.slice.call, but that'd be more verbose. Spreading into an array and then using array methods makes for short, concise, readable code \$\endgroup\$ Sep 22 '20 at 19:01
  • \$\begingroup\$ Where did you learn all this? - you literally answer all my questions on point and great depth. \$\endgroup\$ Sep 22 '20 at 21:18

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.