Problem
I’d be grateful for a review of my AJAX request. If I have taken the correct approach here and especially any security concerns!
I am creating a Purchase Order system for my manager (as part of an apprenticeship I am doing).
There is a select html element where a supplier can be selected. I also have a button to trigger a bootstrap modal with a form to enter a new supplier. I have an AJAX request to add the new supplier to the database then a chain AJAX request to query the database so the select element can be updated.
<script>
window.addEventListener('DOMContentLoaded', function () {
// User can add a new supplier using a modal form.
// 1) Send an AJAX POST request to update the database
// 2) Send AJAX GET request to query the database
// 3) Repopulate the select element with the updated records.
document.getElementById("newSupplierModalForm").addEventListener('submit', function (e) {
// Send AJAX request to add new supplier.
// TODO: Validate name field has text and handle error.
e.preventDefault();
const token = document.querySelector('meta[name="csrf-token"]').getAttribute('content');
const xhr = new XMLHttpRequest();
xhr.open('POST', '/supplier', true);
// Set headers
xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
xhr.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
xhr.setRequestHeader('X-CSRF-Token', token);
//Set paramaters
const params = new URLSearchParams();
params.set("name", document.getElementById('supName').value);
params.set("address_1", document.getElementById('supAddress_1').value);
params.set("address_2", document.getElementById('supAddress_2').value);
params.set("town", document.getElementById('supTown').value);
params.set("post_code", document.getElementById('supPost_code').value);
params.set("contact_name", document.getElementById('supContact_name').value);
params.set("contact_tel", document.getElementById('supContact_tel').value);
params.set("contact_email", document.getElementById('supContact_email').value);
// Logic
xhr.onload = function (e) {
if (xhr.status === 200) {
// Close modal
document.getElementById('addSuppModalClose').click();
// AJAX request for updated supplier records to repopulate select element.
const xhr_getSuppliers = new XMLHttpRequest();
xhr_getSuppliers.open('GET', '/getSuppliers', true);
// Set headers
xhr_getSuppliers.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
xhr_getSuppliers.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
xhr_getSuppliers.setRequestHeader('X-CSRF-Token', token);
// Logic
xhr_getSuppliers.onload = function (e) {
if (xhr.status === 200) {
const data = JSON.parse(xhr_getSuppliers.response);
const selectElem = document.getElementById('supplier');
selectElem.options.length = 0; //reset options
// populate options
for (index in data) {
console.log(data[index], index);
selectElem.options[selectElem.options.length] = new Option(data[index], index);
}
}
};
// Send GET Request
xhr_getSuppliers.send();
} else {
// Show alert if AJAX POST Request does not receive a 200 response.
// I need to write code to feed this back to the user later.
alert(xhr.response);
};
};
// Send POST Request
xhr.send(params.toString());
});
});
</script>
```
Solution
Feedback
The code uses const for variables that don’t need to be re-assigned. This is good because it prevents accidental re-assignment. It also uses === when comparing the status codes, avoiding type coercion. That is a good habit.
There are at least four indentation levels because the onload callbacks are anonymous functions. Some readers of the code would refer to this as “callback hell“, because really there is just one large function here with functions defined inside of it, making it challenging to read. See the first section of the Suggestions below for tips on avoiding this.
The const keyword was added to the Ecmascript Latest Draft in Draft status and standard in the Ecmascript 2015 specification1 so other ecmascript-2015 (AKA ES-6) features could be used to simplify the code.
Suggestions
Callbacks and Nesting/Indentation levels
While most of the examples on callbackhell.com focus on promise callbacks and asynchronous code, the concepts apply here as well. Name the functions and then move the definitions out to different lines. If you need to access variables from outside then you may need to pass them as arguments.
Data Object and repeated DOM lookups
I will be honest – I haven’t encountered the URLSearchParams before. I typically see code that uses FormData. Are all of the input elements inside of a <form> tag? if they are then a reference to that element could be passed to the FormData constructor. Otherwise, do the name attributes match the keys added to params – i.e. the id attributes without the sup prefix? You could iterate over the input elements and add the parameters to params.
Avoid global variables
In the review by dfhwze, The following line is referenced:
for (index in data) {
Without any keyword before index, like var, let or const that variable becomes a global (on window).
For let variable in
EcmaScript6 lets you create a closure in a for loop using let. It is preferred over var, which is preferred over nothing (unless you specifically want to reuse an existing variable declared before the loop).
for (index in data) {
console.log(data[index], index);
selectElem.options[selectElem.options.length] = new Option(data[index], index);
}
for (let index in data) {
console.log(data[index], index);
selectElem.options[selectElem.options.length] = new Option(data[index], index);
}
Semi-colon
If-Else branch does not require a semi-colon to terminate it.
} else {
// Show alert if AJAX POST Request does not receive a 200 response.
// I need to write code to feed this back to the user later.
alert(xhr.response);
};
} else {
// Show alert if AJAX POST Request does not receive a 200 response.
// I need to write code to feed this back to the user later.
alert(xhr.response);
}