Variable declaration outside or inside when using on of the methods of Array.prototype

Question

The purpose of this script is to enforce some rules for validating a keyboard shortcuts definition. In my case I want to reject duplicates keys improperly added in the combo properties. This code rejects key combinations like CtrlAltPPwhich are not valid.

I would like to know what are the best practices regarding variable declaration placement (in/out) when method like Array.prototype.forEach are used.

Which version of the code would you suggest as better? Also, for my understanding in the first snippet, variables declared inside the function in .forEach are hoisted to that function and not to the check() function, is my assumption correct?

window.app = {
    combos: {
        a: 'Ctrl-Alt-P',
        b: 'Ctrl-Ctrl-Alt-P', // invalid
        c: 'Ctrl-Alt-P-P'// invalid
    },
    check: function () {
        var result = true,
            combos = Object.keys(this.combos).forEach(function (combo) {
                var comboWin = this.combos[combo],
                    parts = comboWin.split('-').sort(),
                    item,
                    itemNext;
                for (var i = 0, len = parts.length; i < len; i++) {
                    item = parts[i];
                    itemNext = parts[i + 1];
                    if (item === itemNext) {
                        result = false;
                        alert('DUPLICATED PART IN: '+ comboWin + ' : ' + item);
                    }
                }
            }.bind(this));
        return result;
    }
}

window.app.check();

 window.app = {
        combos: {
            a: 'Ctrl-Alt-P',
            b: 'Ctrl-Ctrl-Alt-P', // invalid
            c: 'Ctrl-Alt-P-P'// invalid
        },
        check: function () {
            var result = true,
                comboWin,
                parts,
                item,
                itemNext;
            combos = Object.keys(this.combos).forEach(function (combo) {
                comboWin = this.combos[combo],
                parts = comboWin.split('-').sort(),
                item,
                itemNext;
                for (var i = 0, len = parts.length; i < len; i++) {
                    item = parts[i];
                    itemNext = parts[i + 1];
                    if (item === itemNext) {
                        result = false;
                        alert('DUPLICATED PART IN: ' + comboWin + ' : ' + item);
                    }
                }
            }.bind(this));
            return result;
        }
    }
    
    window.app.check();

Answer 1

Concerning your variable scope, I think it is safer to define it inside the function as it will assure you get the value inside it. That way you won't be able to access a filled in parts variable after your function - in this case it makes no difference, but once you have a couple more things going on it is best to declare your variables with as little scope as possible. This is because you variables will be cleaned up automatically after your function has ended (and garbage collection has come by), which means you are freeing up memory and not filling it with data you won't use anymore.

I have been able to eliminate almost all your variables because of returns. Instead of storing something when you just need to use it to return, just return it directly. It cuts down on a lot of variables, and a lot of code.

The point is that you don't need to use variables unless you need to use them later. After deciding what variables to use, you have to decide what scope you need. Keep the scope as small as possible, so it's as predictable as possible to see what variable is available where and reduce memory consumption.

window.app = {
    combos: {
        a: 'A-A-B-C-C',
        b: 'A-B-C',
        c: 'A-B-C-C-D'
    },
    check: function () {
        // If like declaring variables at the top, it keeps the rest consistent
        return Object.keys(this.combos).forEach(function(combo){
            // `combo` is already defined, and since you don't use
            // this.combos anymore after this, dont store it in a var
            var parts = this.combos[combo].split('-').sort();
            // Also, I also recently came across this 
            // clever piece on Code Review:
            return parts.forEach(function(part, index){
                // To check duplicates, just check
                // if first index != last index
                if(index != parts.lastIndexOf(part)){
                    document.write('DUPLICATED PART IN: ' + combo + ' : ' + part + '<br />');
                    // You can just return here.
                    // Breaks the loop and returns!
                    return false;
                }
                return true;
            });
        }, this);
        return result;
    }
}
    
document.write('>>>' + (window.app.check() ? 'Completed without duplicates' : 'Duplicates found.'));

I also replaced your alert with document.writes as alerts are just... annoying in my opinion. Check the snippet above to see a nice result.

One more thing: if you do want to bind a this arguments to forEach, it already provides a quick way to do that: the second argument of the forEach function is actually defined as thisArg:

arr.forEach(callback[, thisArg])

Your assumption is correct: variables declared within blocks are scoped only to that block. If, however, you omit the keyword var, they will be avail;able outside your block as they are usually appended to the top-level object in the chain, usually the window.

document.write('Executing a function that uses <code>var</code> to declare local scope.<br />');

+function(){
    var foobar = 'Foo Bar';
}();

document.write(typeof foobar !== 'undefined' ? '>>Foobar is ' + foobar : '>> Foobar is undefined. <br />');

/*-------------------------------*/

document.write('Executing a function that does not use <code>var</code> to declare local scope.<br />');

+function(){
    foobar = 'Foo Bar';
}();

document.write(typeof foobar !== 'undefined' ? '>> Foobar is: ' + foobar : '>> Foobar does not exist <br />');

/* Note: using the + sign is equal to wrapping the function in brackets etc... Its create a temporary scope an executes in it. */