2
\$\begingroup\$

This is basically my first real venture into JS coding ... I think what I have put together here is quite long-winded and could be streamlined but I'm not quite sure how.

So I have a table with Columns "Region", "Country", and "Display".

CREATE TABLE [dbo].[DATABASE](
  [Region] [varchar](20) NOT NULL,
  [Country] [varchar](40) NOT NULL,
  [Display] [varchar](1) NULL
) ON [PRIMARY]

Heres some info from the database

EMEA    Azerbaijan  Y
CALA    Bahamas     Y
EMEA    Bahrain     Y
APAC    Bangladesh  Y
CALA    Barbados    Y
EMEA    Belarus     Y

Region is either EMEA, CALA or APAC.

Country is the name of the countries in that Region

Display is Y if its to be displayed in the list

eg

EMEA / France / Y

EMEA / Italy / Y

APAC / India / Y

the HTML page I would like to display is a selection of regions (countries hidden) and if a region is checked then the countries in that region are displayed with checkboxes so that they can be also selected. The overall idea is to have a list of countries you want to see information on.

You can check an "ALL regions" option and then check an "ALL countries" option to check all that region

Here's the JS

<script>
function ChooseEMEA() {
  var checkEMEA = document.getElementById("checkEMEA");
  var textEMEA = document.getElementById("textEMEA");
  if (checkEMEA.checked == true){
    textEMEA.style.display = "block";
  } else {
    textEMEA.style.display = "none";
    var allCB = document.querySelectorAll("input[class='CouCheckEMEA']");
    var AllEMEAChecked = document.getElementById("checkAllEMEA");
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
    AllEMEAChecked.checked=false;
  }
}

function ChooseAPAC() {
  var checkAPAC = document.getElementById("checkAPAC");
  var textAPAC = document.getElementById("textAPAC");
  if (checkAPAC.checked == true){
    textAPAC.style.display = "block";
  } else {
    textAPAC.style.display = "none";
    var allCB = document.querySelectorAll("input[class='CouCheckAPAC']");
    var AllAPACChecked = document.getElementById("checkAllAPAC");
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
    AllAPACChecked.checked=false;
  }
}

function ChooseCALA() {
  var checkCALA = document.getElementById("checkCALA");
  var textCALA = document.getElementById("textCALA");
  if (checkCALA.checked == true){
    textCALA.style.display = "block";
  } else {
    textCALA.style.display = "none";
    var allCB = document.querySelectorAll("input[class='CouCheckCALA']");
    var AllCALAChecked = document.getElementById("checkAllCALA");
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
    AllCALAChecked.checked=false;
  }
}

function ChooseALL() {
  var checkAll = document.getElementById("checkAll");
  var checkEMEA = document.getElementById("checkEMEA");
  var checkAPAC = document.getElementById("checkAPAC");
  var checkCALA = document.getElementById("checkCALA");
  var textEMEA = document.getElementById("textEMEA");
  var textAPAC = document.getElementById("textAPAC");
  var textCALA = document.getElementById("textCALA");
  if (checkAll.checked == true){
    textEMEA.style.display = "block";
    textAPAC.style.display = "block";
    textCALA.style.display = "block";
    checkAPAC.checked = true;
    checkEMEA.checked = true;
    checkCALA.checked = true;
  } else {
    textEMEA.style.display = "none";
    textAPAC.style.display = "none";
    textCALA.style.display = "none";
    checkAPAC.checked = false;
    checkEMEA.checked = false;
    checkCALA.checked = false;

  }
}

function ChooseALLEMEA() {
  var allCB = document.querySelectorAll("input[class='CouCheckEMEA']");
  var AllEMEAChecked = document.getElementById("checkAllEMEA");
  if (AllEMEAChecked.checked == true){
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=true;
    }
  }
  else{
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
  }
}
function ChooseALLAPAC() {
  var allCB = document.querySelectorAll("input[class='CouCheckAPAC']");
  var AllAPACChecked = document.getElementById("checkAllAPAC");
  if (AllAPACChecked.checked == true){
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=true;
    }
  }
  else{
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
  }
}
function ChooseALLCALA() {
  var allCB = document.querySelectorAll("input[class='CouCheckCALA']");
  var AllCALAChecked = document.getElementById("checkAllCALA");
  if (AllCALAChecked.checked == true){
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=true;
    }
  }
  else{
    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=false;
    }
  }
}

</script>

Here's the HTML:

<?php
include ('./lib/db_config.php'); //DATABASE logon info and connections

$EMEAsql = "SELECT DISTINCT Country
 FROM DATABASE
 WHERE Region='EMEA' AND Display='Y'
 ORDER BY Country  ASC";
$APACsql = "SELECT DISTINCT Country
 FROM DATABASE
 WHERE Region='APAC' AND Display='Y'
 ORDER BY Country  ASC";
$CALAsql = "SELECT DISTINCT Country
 FROM DATABASE
 WHERE Region='CALA' AND Display='Y'
 ORDER BY Country  ASC";

?>
<table width="80%">
  <tr><th>Select your Region</th><th colspan="3">Select your Country</th>  <th></th>
 </tr>
 <tr>
   <td  width="20%" valign="Top" align="Left">

     <form action="./action_page.php" method="POST">
       <p>
         <input type="checkbox" id="checkAll"  onclick="ChooseALL()">   Region All / None<br><br>
         <input type="checkbox" id="checkEMEA" class="checkEMEA"  onclick="ChooseEMEA()"> EMEA<br>
         <input type="checkbox" id="checkAPAC"  onclick="ChooseAPAC()"> Asia Pacific<br>
         <input type="checkbox" id="checkCALA"  onclick="ChooseCALA()"> Canada & Latin America<br>
       </p>
     </td>
     <td  width="20%" valign="Top" align="Left">

       <p id="textEMEA" style="display:none">
         <input type="checkbox" id="checkAllEMEA" class="checkAllEMEA"   onclick="ChooseALLEMEA()"> EMEA All / None<br><br>
         <?php   if (($result = sqlsrv_query($conn, $EMEAsql)) !== false) 
         {
           while ($obj = sqlsrv_fetch_object($result))
           {
             $Country = ($obj->Country);
             echo '<input type="checkbox" class="CouCheckEMEA"  name="country[]" value="'.$Country.'" onclick="countrySelect(this.value)">'.$Country.'<br>';
           }
         }
         ?>

       </p>
     </td>
     <td  width="20%" valign="Top" align="Left">
       <p id="textAPAC" style="display:none">
         <input type="checkbox" id="checkAllAPAC" name="checkAllAPAC"   onclick="ChooseALLAPAC()"> Asia Pacific All / None<br><br>
         <?php   if (($result = sqlsrv_query($conn, $APACsql)) !== false)
         {
           while ($obj = sqlsrv_fetch_object($result))
           {
             $Country = ($obj->Country);
             echo '<input type="checkbox" class="CouCheckAPAC"   name="country[]" value="'.$Country.'" onclick="countrySelect(this.value)">'.$Country.'<br>';
           }
         }
         ?>
       </td>
       <td  width="20%" valign="Top" align="Left">
       </p>
       <p id="textCALA" style="display:none">
         <input type="checkbox" id="checkAllCALA" name="checkAllCALA"   onclick="ChooseALLCALA()"> Canada & Latin America All / None<br><br>
         <?php   if (($result = sqlsrv_query($conn, $CALAsql)) !== false)
         {
           while ($obj = sqlsrv_fetch_object($result))
           {
             $Country = ($obj->Country);
             echo '<input type="checkbox" class="CouCheckCALA"   name="country[]" value="'.$Country.'" onclick="countrySelect(this.value)">'.$Country.'<br>';
           }
         }
         ?>
       </p>
     </td>
     <td  width="20%" valign="Top" align="Left">
       <button type="submit">Submit form</button>
     </td>
   </tr>
 </form>

Am I over complicating my JavaScript functions?

I'm wondering if using DIV elements would be a smarter way of working this and if the JS can be consolidated in some way

\$\endgroup\$
2
\$\begingroup\$

You're repeating a lot of code that you could greatly condense with a little extra work.

An excellent rule to follow is DRY: don't repeat yourself. If you see large blocks of code that repeat two or more times, consider condensing and implementing variables to keep the functionality.

In your code, you're repeating everything inside the ChooseEMEA, ChooseAPAC, and ChooseCALA functions. Since the only thing that changes between these is the region, you can pass the region as a parameter and use that to differentiate between them.

// pass in region name as parameter
function choose(region) {
  // concatenate region name into ids
  var checkbox = document.getElementById("check" + region);
  var text = document.getElementById("text" + region);
  // checkbox and text can now be referenced without region name
  if (checkbox.checked) {
    text.style.display = "block";
  } else {
    text.style.display = "none";
    // concatenate region name with "CouCheck" using an embedded expression
    var allCB = document.querySelectorAll(`input[class=CouCheck${region}]`);
    var allChecked = document.getElementById("checkAll" + region);
    for(var i=0; i<allCB.length; i++){
      allCB[i].checked = false;
    }
    allChecked.checked = false;
  }
}

// use the function with each region name
choose("EMEA");
choose("APAC");
choose("CALA");

Additional comments:

  1. Function names and plain variables are commonly typed in lower camel case.
  2. To check a value's truthiness in an if-statement, it's easier (and often more readable) to simply state the variable without a comparison (e.g., === true) following it. I made this change in the code above.
  3. When checking equality, it is good practice to always use strict equality unless you know the values may not be of the same type.
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Thanks for contributing this nice post; I hope to see more of your contributions in future here in our community! \$\endgroup\$ – Toby Speight Aug 21 '18 at 16:39
  • \$\begingroup\$ This is exactly what I was looking for - thanks very much for your insight and input here - Ill implement this moving forward.. \$\endgroup\$ – Malcolm H Aug 22 '18 at 8:49
0
\$\begingroup\$

I concur with @adam - in his answer the main point is correct: there is a lot of redundancy and the D.R.Y. principle should be respected. Not only does it apply to the JavaScript but also the PHP & SQL code.

I would suggest doing one single query at the start of the script to get all records of countries along with Region, ordering by Region:

$sql = "SELECT DISTINCT Region, Country
FROM DATABASE
WHERE Display='Y'
ORDER BY Region ASC, Country  ASC";

Then group the data by Region:

$dataByRegion = array();

if (($result = sqlsrv_query(true, $sql)) !== false)
{
    while ($obj = sqlsrv_fetch_object($result)) 
    {
        $dataByRegion[$obj->Region][] = $obj;
    }    
}

That way when creating table cells, you can check if each region has country records to display using array_key_exists() and count():

<?php
foreach(array('EMEA', 'APAC', 'CALA') as $region) //could also use array_keys($dataByRegion)
{
    if (array_key_exists($region, $dataByRegion) && count($dataByRegion[$region])) 
    {
?>    
 <td  width="20%" valign="Top" align="Left">
     <p id="text<?=$region?>" class="region <?=$region?>" style="display:none">
     <input type="checkbox" class="checkAll"> <?=$region?> All / None<br><br>
     <?php   
       foreach ($dataByRegion[$region] as $obj)
       {
         $Country = ($obj->Country);
         echo '<input type="checkbox" class="CouCheck '.$region.'"  name="country[]" value="'.$Country.'" >'.$Country.'<br>';
       } //end foreach obj
     } //end if array_key_exists && count
     ?>
   </p>
 </td> 
<?php
}

Then in the Javascript code:

  1. Wait for the DOM to be ready - use document.addEventListener() to wait for the DOMContentLoaded event:

    document.addEventListener('DOMContentLoaded', function() {
        //add event delegate
    });
    
  2. Take out code from HTML e.g. onclick and add event handlers in Javascript - e.g. document.body.addEventListener('click', function(event) { } );. That way if a function name and/or signature changes, only other Javascript code will need to be updated (hopefully in one spot), instead of needing to update a bunch of elements in the HTML.
  3. Use data attributes or class names to add the region names and bind those on the checkboxes for the countries.
  4. Instead of using document.querySelectorAll() just to get elements by class name, as in

    var allCB = document.querySelectorAll("input[class='CouCheckAPAC']");
    

    one could use document.getElementsByClassName() which would be quicker. But instead of querying the DOM, one could also utilize the form property document.forms at index 0 (unless there are multiple forms- then use whatever index is necessary or a name) and then the .elements property. I would suggest selecting all country checkboxes at the start of the Javascript and then when needed, filtering to the checkboxes necessary. For that, use Array.from() to get an array from document.forms[0].elements['country[]']

    var countryCheckboxes = Array.from(document.forms[0].elements['country[]']);
    
  5. Eliminate other repeated code- e.g.:

    if (AllEMEAChecked.checked == true){
      for(var i=0; i< allCB.length; i++){
        allCB[i].checked=true;
      }
    }
    else{
      for(var i=0; i< allCB.length; i++){
        allCB[i].checked=false;
      }
     }
    

    Utilize AllEMEAChecked.checked inside the loop and just use one loop:

    for(var i=0; i< allCB.length; i++){
      allCB[i].checked=AllEMEAChecked.checked;
    }
    

See This PHP playground example for a starter rewrite - the select all checkboxes above each region aren't working but that can be an exercise to the reader.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much for your time and efforts in taking a look at this... Much appreciated. I'll spend time looking into your comments above and implement where needed. However I will say that it may be a trade off between consolidating the code and therefore less easy to read and leaving it more open for others to read and add to where needed.... thanks again. \$\endgroup\$ – Malcolm H Aug 22 '18 at 8:47

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.