0

Can anyone provide feedback on how i can improve the script below? The script does work properly but uses Global Variables, I've been told that the use of global variables can cause issues in code.

    var vehicle = document.getElementById('vehicle');
    var residence = document.getElementById('residence');

    vehicle.setAttribute("class", "hide");

    document.getElementById('myList').addEventListener('change', function(){
        Array.prototype.forEach.call(document.querySelectorAll('.forms'), function(e){
            e.setAttribute("class", "hide");
        });

        var sel=+this.selectedIndex - 2;
        if(sel >= 0){
            vehicle.setAttribute("class", "show");
            residence.setAttribute("class", "hide");
        } else {
            residence.setAttribute("class", "show");
            vehicle.setAttribute("class", "hide");
        }
    });
Improve this question
3
  • depends, how is this code being used? What is the application? If there is something you don't want your users snooping into you should look into minifying your javascript code. Commented Feb 3, 2014 at 22:21
  • 2
    Why are you using e.setAttribute("class", "hide"); instead of e.className = "hide";? Commented Feb 3, 2014 at 22:21
  • @Mike Minifying really doesn't help that much in terms of hiding content from users. Simply copy the code and paste it into an "unminifier" such as this, and your code is fully readable again. Commented Feb 3, 2014 at 22:23

4 Answers 4

Reset to default
7

use a function for privacy:

<script type="text/javascript">
(function(){

    var vehicle = document.getElementById('vehicle');
    var residence = document.getElementById('residence');

    vehicle.setAttribute("class", "hide");

    document.getElementById('myList').addEventListener('change', function(){
        Array.prototype.forEach.call(document.querySelectorAll('.show'), function(e){
            e.setAttribute("class", "hide");
        });

        var sel=+this.selectedIndex - 2;
        if(sel >= 0){
            vehicle.setAttribute("class", "show");
        } else {
            residence.setAttribute("class", "show");
        }
    });

})();
</script>

doesn't get much easier: you don't need to alter your written code at all, just wrap it up.

Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

@War10ck why? and @dandavis if you want to improve this script at first you should change vehicle.setAttribute("class", "hide"); to vehicle.className = "hide"; and than instead of Array.prototype.forEach.call you can simply use [].forEach.call, also get all variable declarations at the top, and add strict mode. ;)
i wanted to demonstrate the ease of implementing a wrapper function, and basically preserving the orig code (for better or worse) makes that simplicity crystal clear. Otherwise, it could be confusing what change actually helped.
@Givi It's the proper syntax. You're closing the anonymous function with this }) and then immediately invoking it via a function call like (); this. The way it was written originally should have thrown a syntax error.
@War10ck but JSLint throws error: Move the invocation into the parens that contain the function.
lint and hint are not for the same purpose. lint is for checking human code readability, hint is for checking machine code quality.
|
1

It would be way better to modularize your code into testable and reusable components.

There are tons of ways to achieve this, but here's a simple example...

Define an object that represents your feature:

yourNS = window.yourNS || {};

yourNS.YourFeature = {
    init: function (options) {
        var vehiculeEl = document.querySelector(options.vehicleEl),
            residenceEl = document.querySelector(options.residenceEl),
            listEl = document.querySelector(options.listEl);

        vehiculeEl.className = 'hide';

        listEl.addEventListener('change', function () {

            var showVehicule = this.selectedIndex - 2 >= 0;

            vehiculeEl.className = showVehicule? 'show' : 'hide';
            residenceEl.className = showVehicule? 'hide' : 'show';
        });
    }
};

Use it:

!function () {
    yourNS.YourFeature.init({
        vehiculeEl: '#vehicule',
        residenceEl: '#residence',
        listEl: '#myList'
    });
}();

You might be interested by Writing Testable JavaScript, written by Rebecca Murphey.

Improve this answer

Comments

0

Everyone has it's own JS style, but I prefer using a global JS object that stores all my variables but this only where it is really needed.

var GLOBAL = 
{
    key:"value",
    key2:variable,
    key3:
    {
        key4:variable,
        key5:'hans'
    }
};
GLOBAL.key3.key5 = 'peter';

An other option is to use a function that does everything, so the variables you use are contained in the namespace of your function.

var foo = function()
{
    var iAmInvisible = 42;
}
Improve this answer

1 Comment

who you calling "it"? ;)
0

For global variables you can use object "window":

window.vehicle = { param1 : "abc", param2 : document.getElementById('idXYZ').value};  // It is not necessary "var"

to access the variable :

window.vehicle.param1
Improve this answer

Comments

Your Answer

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