2
\$\begingroup\$

I'm a junior developer. And I want to learn the good way to code. The code I will show you is working, but I feel like its all messy and longer that it should be. It is simply a .js, a .html and some css that render a list of informations about scans. The list come from Firebase (Google Analytics).

Here is the code I suspect of being messy - it's a script that sort the 'li' components of the list each time a date is entered or a selection is made (or both).

<script>

    function myFunction() {
        // Declare variables
        var input, input2, filter, filter2, ul, li, a, i, txtFirstValue, txtThirdValue, txtFourthValue;
        input = document.getElementById('myInput');
        input2 = document.getElementById('myInput2');
        filter = input.value.toUpperCase();
        filter2 = input2.value.toUpperCase();
        ul = document.getElementById("Scan-list");
        li = ul.getElementsByTagName('li');
        counter = 0;
        test = li[0].getElementsByTagName("span")[3];

          // Loop through all list items, and hide those who don't match the search query (date 1 to date 2)
          for (i = 0; i < li.length; i++) {
            scanFirstValue = li[i].getElementsByTagName("span")[0];
            scanThirdValue = li[i].getElementsByTagName("span")[2];
            scanFourthValue = li[i].getElementsByTagName("span")[3];
            txtFirstValue = scanFirstValue.textContent;
            txtThirdValue = scanThirdValue.textContent;
            txtFourthValue = scanFourthValue.textContent;
            var stringToDate = new Date(txtFirstValue);
            var testSelectBox = dropdownBodypartsDeep.value;
            var testvaluebiddon = "Scan Type Specialize : Dorsum";

            if (input.value == ""){
                if(

                ((txtThirdValue == "Scan Type : " + dropdownBodyparts.value)
                &&
                (txtFourthValue == "Scan Type Specialize : " + dropdownBodypartsDeep.value))

                |

                ((txtThirdValue == "Scan Type : " + dropdownBodyparts.value)
                &&
                (txtFourthValue == "Scan Type Specialize : "))

                ) 
                {
                    li[i].style.display = "";
                    counter = counter +1;
                }
                else if (dropdownBodyparts.value == "All"){
                    li[i].style.display = "";
                    counter = counter +1;
                }
                else if  (
                    (txtThirdValue == "Scan Type : " + dropdownBodyparts.value)

                    &&

                    (dropdownBodypartsDeep.value == "All")){
                        li[i].style.display = "";
                        counter = counter +1;
                    }

                else {
                    li[i].style.display = "none";
                }
            }

            else if(input.value != "" && dropdownBodyparts.value == "null") {
                if(
                (stringToDate.getTime() >= (new Date(input.value)).getTime() && stringToDate.getTime() <= (new Date(input2.value)).getTime()) 
                ) 
                {
                    li[i].style.display = "";
                    counter = counter +1;
                }
                else {
                    li[i].style.display = "none";
                }
            }

            else if(input.value != "" && dropdownBodyparts.value != "null") {
                if(
                (stringToDate.getTime() >= (new Date(input.value)).getTime() && stringToDate.getTime() <= (new Date(input2.value)).getTime()) 
                &&
                ((txtThirdValue == "Scan Type : " + dropdownBodyparts.value)
                &&
                (txtFourthValue == "Scan Type Specialize : " + dropdownBodypartsDeep.value))

                |

                ((txtThirdValue == "Scan Type : " + dropdownBodyparts.value)
                &&
                (txtFourthValue == "Scan Type Specialize : "))
                ) 
                {
                    li[i].style.display = "";
                    counter = counter +1;
                }
                else if (dropdownBodyparts.value == "All"
                        &&
                        (stringToDate.getTime() >= (new Date(input.value)).getTime() && stringToDate.getTime() <= (new Date(input2.value)).getTime()))
                {
                    li[i].style.display = "";
                    counter = counter +1;
                }
                else {
                    li[i].style.display = "none";
                }
            }

        }
        document.getElementById('counterLabel').innerHTML = counter;
    }

</script>

And here is the HTML for the web page :

<body>

<h1 id="titleLabel" >TechMed3D Scan DataBase</h1>

<div id="imageTechMed">
    <img src="css/3dsizeme2019.png" class="topRight" style="width:250px;height:45px;">
</div>

<div class="content">

    <div class="container">
        <label id="labelDate">Date de départ : </label>
        <input type="text" id="myInput" name="a" onkeyup="myFunction()" placeholder="yyyy-mm-dd" >
        <form>
            <select name = "dropdown" id="dropdownBodyparts" onchange="if (this.selectedIndex) myFunction();">
               <option disabled selected value = "null"> -- Select A Bodypart --</option>
               <option value = "All" >All</option>
               <option value = "Head">Head</option>
               <option value = "Foot">Foot</option>
               <option value = "Leg">Leg</option>
               <option value = "Elbow">Elbow</option>
               <option value = "Torso">Torso</option>
            </select>
         </form>
    </div>
    <div class="container">
        <label id="labelDate">Date de fin : </label>
        <input type="text" id="myInput2" name="b" onkeyup="myFunction()" placeholder="yyyy-mm-dd" >
        <form>
            <select name = "dropdown" id="dropdownBodypartsDeep" onchange="if (this.selectedIndex) myFunction();">
               <option value = "Specialized" selected>Specialized</option>
            </select>
         </form>
            <div id="scanNumberRight">
                <label id="counterAnouncerLabel">Nombre de scan dans la liste : </label>
                <label id="counterLabel">-</label>
            </div>
    </div>

    <div style="text-align:center;">
        <input type="button" id="first" onclick="firstPage()" value="first" />
        <input type="button" id="next" onclick="nextPage()" value="next" />
        <input type="button" id="previous" onclick="previousPage()" value="previous" />
        <input type="button" id="last" onclick="lastPage()" value="last" />
    </div>

    <ul id="Scan-list"></ul>

</div>

For the selectBox, i use this javascript code to render the good value :

//Set the second selectBox depending on the first one choice.
$(document).ready(function () {
    $("#dropdownBodyparts").change(function () {
        var val = $(this).val();
        if (val == "All") {
            $("#dropdownBodypartsDeep").html("<option value='All'>All</option>");
        } else if (val == "Head") {
            $("#dropdownBodypartsDeep").html("<option value='Specialized'>Specialized</option>");
        } else if (val == "Foot") {
            $("#dropdownBodypartsDeep").html("<option disabled selected value> -- Select A Specialized Bodypart</option><option value='All'>All</option><option value='Dorsum + Imprint'>Dorsum + Imprint</option><option value='Plantar Surface'>Plantar Surface</option><option value='Foam Box'>Foam Box</option><option value='Dorsum'>Dorsum</option><option value='for AFO'>for AFO</option>");
        } else if (val == "Leg") {
            $("#dropdownBodypartsDeep").html("<option disabled selected value> -- Select A Specialized Bodypart</option><option value='All'>All</option><option value='Knee'>Knee</option><option value='AK'>AK</option><option value='BK'>BK</option>");
        } else if (val == "Elbow") {
            $("#dropdownBodypartsDeep").html("<option value='Specialized'>Specialized</option>");
        } else if (val == "Torso") {
            $("#dropdownBodypartsDeep").html("<option disabled selected value> -- Select A Specialized Bodypart</option><option value='All'>All</option><option value='Normal'>Normal</option><option value='Two-sided'>Two-sided</option><option value='Mirror'>Mirror</option><option value='Seating'>Seating</option>");
        }
    });
});

And here is the UI

Image

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Use meaningful variable names (and element IDs), for example, startDateInput instead of input/myInput.

Declare varables in the smallest needed scope instead all of them at the start of the function, and declare each variable in a separate statement. Use let or const instead of var (unless you need to support enviroments that don't support them).

Move the variables that don't change for each execution of the function, such as the input references, outside the function.

Use the children property to access the list items of the scan list.

Example of these changes:

const startDateInput = document.getElementById('startDateInput');
const endDateInput = document.getElementById('endDateInput');
const scanList = document.getElementById("Scan-list");

function myFunction() {
    const startDateFilter = startDateInput.value.toUpperCase();
    const endDateFilter = endDateInput.value.toUpperCase();
    const li = scanList.children;
    let counter = 0;

    // ...
}

Unless you exlipictly need the index use a for ... of loop to iterate over the lis.

Don't repeat things like getElementsByTagName("span").

for (const item of li) {
    const spans = item.getElementsByTagName("span")
    const itemDateString = spans[0].contentText; // Variable name describing content
    const itemBodyPart = spans[2].contentText;

    // ...
 }

Be careful using new Date(...) to convert arbitrarily strings into dates. Especially it will break if you use a different date format in the output.


The big filtering if block is difficult to read. Don't repeat the same comparisons. Extract the large boolean expressions that go over multiple lines into separate functions.


Don't hard code strings such as "All" and "Scan Type : " in your code. It makes translations diffcult.

Generally parsing the data out of the output HTML is a bad idea. It would be better to filter the raw data that the list items represent instead, but that would require completely different code.

At least consider encoding the relevant data in the list items in microdata attributes or data- attributes.

For example, instead of

<li> 
   <span>2019-12-24</span>
   <span>Scan Type : Foot</span>
</li>

have something like this (with microdata):

 <li itemscope>
     <!-- Identify the elememt via the `itemprop` attribute instead of its index.
          Have the datetime attribute in a computer readable format and display
          the date to the user in a human readable format -->
     <time itemprop="date" datetime="2019-12-24">
       December 24th, 2019
     </time> 
     <!-- `value` attribute contains the data in format the script understands
          and the text is translatable and human readable. -->
     <span itemprop="body-part" value="Foot">
        Scan Type : Pied
     </span>
  </li>

Or with data attributes:

 <li>
     <span data-date="2019-12-24">
       December 24th, 2019
     </span>
     <span data-body-part="Foot">
        Scan Type : Pied
     </span>
  </li>

counter = counter +1; can be simplified to counter++.

Don't repeat li[i].style.display = ""; counter = counter +1;. Instead have your filtering if block return a boolean (visible), and then at the end say:

if (visible) {
    li[i].style.display = "";
    // Or `item.style.display = "";` in my example

    counter++;
} else {
    li[i].style.display = "none";
}

Don't assign event handlers in the HTML in the on... attributes. Instead assign them in the JavaScript code with addEventListener.

disabled and selected on the same option doesn't make much sense.


You didn't use jQuery in myFunction so no need to start using it in the other JavaScript. Avoid jQuery as much as you can, especially in such simple scripts where its features aren't taken advantage of.

\$\endgroup\$
1
  • \$\begingroup\$ First,@RoToRa I want to thank you for your time and all the help you gave me. I didnt catch all the info you gave me but it helped me a lot and now i feel like my code is more robust and cleaner. I still have to learn things to make the changes you suggest me. For exemple : i cant change my for loop to a " for(const item of li)" because i cant display my item afterward... Thanks again for your knowledge, I learned a lot! \$\endgroup\$ Commented Jan 6, 2020 at 21:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.