Node JS and Handlebars checking if records have been completed today

Question

Goal:

Currently creating an application using Node JS and Handlebars.

I have a to do list set up in Handlebars, when clicked on Start today's report, it shows up the to do list:

 <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css" integrity="sha384-JcKb8q3iqJ61gNV9KGb8thSsNjpSL0n8PARn9HuZOnIxN0hoP+VmmDGMN5t9UJ0Z" crossorigin="anonymous">
<section id="daily-reports">
 <div class="container">
 <div class="jumbotron">
 <h1 class="display-4">Daily Reports</h1>
 <p class="lead">View completed reports or start daily reports.</p>
 <button type="button" class="btn btn-primary" data-toggle="modal" data-target="#dailyModal">
 Start today's report
 </button>
 <button type="button" class="btn btn-primary">
 View Data
 </button>
 </div>
 </div>
 <!-- Button trigger modal -->
 <!-- Modal -->
 <div class="modal fade" id="dailyModal" tabindex="-1" role="dialog" aria-labelledby="dailyModalLabel" aria-hidden="true">
 <div class="modal-dialog" role="document">
 <div class="modal-content">
 <div class="modal-header">
 <h5 class="modal-title" id="dailyModalLabel">To do</h5>
 <button type="button" class="close" data-dismiss="modal" aria-label="Close">
 <span aria-hidden="true">&times;</span>
 </button>
 </div>
 <div class="modal-body">
 <div class="container">
 <div class="row">
 <div class="col">
 <a id="tempBtn" href="/temp-check">Temperature</a>
 </div>
 <div class="col">
 <input id="temp" type="checkbox" name="" value="" disabled {{temp}}>
 </div>
 </div>
 </div>
 </div>
 <div class="modal-footer">
 <button type="button" class="btn btn-secondary" data-dismiss="modal">Close</button>
 </div>
 </div>
 </div>
 </div>
</section>
 <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/[email protected]/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>

Here is the Node JS server side to determine whether to check or un-check the checkbox

const express = require('express');
const router = express.Router();
const db = require('../controllers/db').db;
router.get('/', (req, res) => {
 db.query("SELECT DATEDIFF(time, curdate()) as lastChecked FROM tempcheck ORDER BY id DESC LIMIT 1;",(error, result) => {
 console.log(result[0].lastChecked);
 if(result[0].lastChecked == 0){
 res.render('home/index', {temp: 'checked'});
 }
 else{
 res.render('home/index', {temp: ''});
 }
 });
});
module.exports = router;

This will affect this handlebars line:

<input id="temp" type="checkbox" name="" value="" disabled {{temp}}>

Next, I have added Javascript script bottom of handlebars, to do the following:

• If checkbox is checked then prompt with a message "You have already completed this check, do you want to do it again?". Like so...

enter image description here

If clicked Ok then continue to form, If clicked Cancel then stop rendering the form.

Here is the Javascript code bottom of handlebars:

...
<script type="text/javascript">
 if(document.getElementById('temp').checked == true){
 document.getElementById('tempBtn').addEventListener('click', function(e){
 if(confirm("You have already completed this check, do you want to do it again?")){
 }
 else{
 e.preventDefault();
 }
 });
 }
</script>

Summary

I'd like to hear some reviews to my current logic, structure and my coding.

Would like to see where I can improve :)

Answer 1

There is:

const db = require('../controllers/db').db;

When the variable name to assign a property to is the same as the property, you can destructure if you wish:

const { db } = require('../controllers/db');

You have:

db.query("SELECT DATEDIFF(time, curdate()) as lastChecked FROM tempcheck ORDER BY id DESC LIMIT 1;",(error, result) => {

There are a number of issues here. First, it's hard to read. Consider defining the query on its own line:

const lastCheckedQuery = `
 SELECT DATEDIFF(time, curdate()) as lastChecked
 FROM tempcheck
 ORDER BY id DESC
 LIMIT 1;
`;

Then, you can pass that in to .query instead.

It depends on what your database is, but since you're only expecting one row to be returned, consider using a method which returns only one row instead - perhaps using .one instead of .query. That way, rather than result[0].lastChecked, you can do something like row.lastChecked.

It's generally easier to work with Promises than with callbacks. This will become especially clear if you ever need to make more than one query before finishing - nested callbacks are ugly, and their logic can be hard to deal with. Again, it depends on your database driver, but consider looking for how to get it to return Promises instead of using callbacks. (.query may already return a Promise)

If you stick with the callback route, at least check whether there's an error before examining the result. The main point of having the callback form having the error first:

(error, result) => {

is to try to force the programmer to think about the error, rather than just ignore it. (Consider using a linter and a rule like no-unused-vars) You'd at least want something like

(error, result) => {
 if (error) {
 // put error into logs / examine it to send more detail to the user
 return res.status(500).send('Unexpected database error');
 }
 // proceed to work with result

Your res.render calls are nearly the same, all that's different is the temp property. This would be done much more concisely with the conditional operator.

The temp property is not very informative. Consider naming it something more meaningful, like possiblyChecked, or a boolean temperatureHasBeenChecked.

You're using == in a couple places. Best to always use === - == has lots of weird rules and gotchas that script-writers should not have to memorize (nor that script-writers should force others who may need to look over their code to have to understand first).

In full, your backend code might be able to look something like:

const express = require('express');
const router = express.Router();
const { db } = require('../controllers/db');
const lastCheckedQuery = `
 SELECT DATEDIFF(time, curdate()) as lastChecked
 FROM tempcheck
 ORDER BY id DESC
 LIMIT 1;
`;
router.get('/', (req, res) => {
 db.one(lastCheckedQuery)
 .then((row) => {
 res.render('home/index', { possiblyChecked: row.lastChecked === 0 ? 'checked' : '' });
 })
 .catch((error) => {
 // put error into logs / examine it to send more detail to the user
 return res.status(500).send('Unexpected database error');
 });
});
module.exports = router;

If checkbox is checked then prompt with a message "You have already completed this check, do you want to do it again?". Like so...

It would be better to avoid prompt if possible. It's pretty user-unfriendly and prevents anything else on the page (such as JavaScript) from running, which is a problem on anything but the most trivial pages. Better to use a proper non-blocking modal instead.

Rather than

if(document.getElementById('temp').checked == true){

Since checked returns a boolean, simply check that boolean:

if (document.getElementById('temp').checked) {

You have:

<a id="tempBtn" href="/temp-check">Temperature</a>

<input id="temp" type="checkbox" name="" value="" disabled {{temp}}>

Personally, I prefer to avoid IDs since they implicitly create global variables. Regardless, tempBtn and temp aren't great names, especially if the page ever gets more complicated - one looking at the code might initially think "Does temp mean "temporary" or "temperature", or something else? And if tempBtn is a button, why is temp called just temp rather than tempCheckbox or something?"

Answer 2

This code has a bit of redundancy:

 if(result[0].lastChecked == 0){
 res.render('home/index', {temp: 'checked'});
 }
 else{
 res.render('home/index', {temp: ''});
 }

The only thing that changes in the two cases is the value of temp in the object.

While only shorter by one line, the following re-written code only has a single line with a call to res.render():

const returnObj = {temp: ''};
if(result[0].lastChecked === 0){
 returnObj.temp = 'checked';
}
res.render('home/index', returnObj);

A benefit of this would be that if the route home/index needed to be updated it could be done in one spot instead of two (or multiple if the code ever got expanded).

It could also be simplified using a ternary operator :

 const returnObj = {temp: result[0].lastChecked === 0 ? 'checked' : '' };

---

In the answer by CertainPerformance the following advice is contained:

Consider using a linter...

That is great advice.

Let's try using eslint with that last block of JavaScript "bottom of handlebars"

<script type="text/javascript">
 if(document.getElementById('temp').checked == true){
 document.getElementById('tempBtn').addEventListener('click', function(e){
 if(confirm("You have already completed this check, do you want to do it again?")){
 }
 else{
 e.preventDefault();
 }
 });
 }
</script>

Under the Messages tab on the right side we see:

3:90 - Empty block statement.no-empty

That link no-empty leads to a page that explains why this rule exists:

Empty block statements, while not technically errors, usually occur due to refactoring that wasn't completed. They can cause confusion when reading code.

That code can be cleaned up (A.K.A. "linted") by reversing the logic on the conditional:

if(!confirm("You have already completed this check, do you want to do it again?")){
 e.preventDefault();
}

That is more concise and would hopefully avoid anybody reading the code (including your future self) from wondering if something should happen in that empty block.

• DeveloperLV: For your first method of using res.render('home/index', returnObj); I am not sure if that is correct. This is because it does not give me any output. Whereas if I did res.render('home/index', {returnObj : returnObj.temp}); it would work.
• Sᴀᴍ Onᴇᴌᴀ: Hmm... did you use the suggestion of using ===? If so, perhaps the expected value is not actually 0 but maybe a string like "false"...