Is there a more efficient way to code this “Item-picker”?

Question

I would love to show you one of my bigger scripts to improve my technique. This script is some kind of Itempicker. You choose your Item first (toprow), then pick a color. The color of the active Item (basically the image) changes.

A big deal for me was actually to design the script that way. The way that all the Items keep their selected color. Further the whole script shall allow to easily add new Items. I think this flexibility results in some more complex code, do you think I've found a good trade-off?

Here is the live example

And here is the actual code of the Itempicker:

$(document).ready( function() {

    var candleAmount = 15; // Anzahl der Kerzen
    var colorAmount = 21;
    var candlePath = "images/kerzen/"; // relativer Pfad zu den Kerzen.png's
    var candleFormat = ".png"; // Bildformat der Kerzen.png's
    var singleImgCandles = [07, 09]; // Kerzen-Nr mit nur einer Farbvariante
    var leftOutColor = [10]; // Kerzenfarben die ausgelassen werden sollen
    $.globalEval("setVisibleRangeDone = false;");

    for (var i=1; i <= candleAmount; i++) { // erstmaliges Erstellen der Kerzen-HTML Elemente samt Array-Erstellung
        var i = correctIndex(i);
        $("#container #candlePick").append("<li class='candle"+i+"'></li>");
        var color = randomColor();
        window["candle" + (i)] = {
            "kind":i, 
            "color": color // i für alle Farben hintereinander
        }
    };

    for (var i=1; i <= colorAmount; i++) { // erstmaliges Erstellen der Farbwahl-HTML Elemente
        var i = correctIndex(i);
        if ($.inArray( eval(i), leftOutColor ) !== -1) { // Kerzefarbe auslassen
            $("#container #colorPick").append("<li><a class='hide color"+i+"' href='#'></a></li>");
        } else {
            $("#container #colorPick").append("<li><a class='color"+i+"' href='#'></a></li>");
        };
    };

    function randomColor () { // farbe wird anhand von colorAmount.length erzeugt
        a = colorAmount;
        b = Math.random();
        color = Math.ceil(b/(1/a))
        color = correctIndex(color);
        if ($.inArray( eval(color), leftOutColor ) !== -1) {
            randomColor ();
        }
        return color;
    };

    function set() { // listenelemente werden mit zugewiesenen Hintergrundbildern versehen
        $("#container #candlePick li").each( function() {
            var i = getIndex(this);
            kind = eval("candle"+i+".kind");
            color = eval("candle"+i+".color");
            if ( $.inArray( eval(kind), singleImgCandles ) !== -1) { // wenn die Kerze nur eine Farbvariante (oben definiert) hat
                color = "01";
            };
            $(this).css("backgroundImage", "url("+candlePath+"kerze-"+color+"-"+kind+candleFormat+")");
        });
    };

    function getIndex(_this) { 
        var i = ($(_this).index())+1; 
        var i = correctIndex(i);
        return i;
    };

    function correctIndex(i) { // ist "i" einstellig, wird eine 0 vor "i" gestellt
        if (i<"10") i="0"+i;
        return i;
    };

    function setVisibleRange() {
        if (setVisibleRangeDone !== true) {
            var fittingAmount = fitIn("#candlePick li");
            $.globalEval("visibleRange = [0, ("+fittingAmount+")-1];");
            $.globalEval("setVisibleRangeDone = true;");
        };
        $("#candlePick li").hide();
        for (i=visibleRange[0]; i<=visibleRange[1]; i++) {
            $("#candlePick li").eq(i).show();
        };
    };

    $(".candleNav.next").click( function() {
        if (visibleRange[1]!==candleAmount-1) {
            $.globalEval("visibleRange[0] += 1")
            $.globalEval("visibleRange[1] += 1")
            setVisibleRange();
        };
    });

    $(".candleNav.prev").click( function () {
        if (visibleRange[0]!==0) {
            $.globalEval("visibleRange[0] -= 1");
            $.globalEval("visibleRange[1] -= 1");
            setVisibleRange();
        };
    });

    function fitIn(element) { // Es wird zurückgegeben, wie oft ein Element von der Breite in sein Elternelement passt
        var ownWidth = ($(element).width());
        var ownBorder = 2; // muss per hand eingetragen werden.
        var ownMargin =  eval($(element).css("marginLeft").slice(-0, -2)) + eval($(element).css("marginRight").slice(-0, -2));
        var ownPadding = eval($(element).css("paddingLeft").slice(-0, -2)) + eval($(element).css("paddingRight").slice(-0, -2));
        var ownTotalWidth = ownWidth+ownMargin+ownPadding+ownBorder;
        var parentWidth = $(element).parent().width();
        var fits = Math.floor((parentWidth/ownTotalWidth));
        return fits;
    };

    function center(element) { // Element wird in seinem Elternelement horizontal zentriert (padding, margin vernachlässigt!)
        var ownWidth = $(element).width();
        var parentWidth = $(element).parent().width();
        var marginLeft = (parentWidth-ownWidth)/2;
        $(element).css("marginLeft", +marginLeft+"px");
    ;}

    $("#container #candlePick li").click( function() {
        $("#container #candlePick li").removeClass("active"); // entfernt bei allen .active
        $(this).addClass("active"); // fügt bei diesem .active hinzu
        var i = getIndex(this);
        color = (eval("candle"+i+".color"))-1;
        $("#container #colorPick li a").removeClass("active");
        $("#container #colorPick li a:eq("+color+")").addClass("active");
        $("#candle").val(i);
        color = eval("candle"+i+".color"); // nur für Einbau, Übergabe an Formular
        $("#color").val(color); // nur für Einbau, Übergabe an Formular

        if ( $.inArray( eval(i), singleImgCandles ) !== -1) { // wenn die Kerze nur eine Farbvariante (oben definiert) hat
            $("#colorPick").fadeTo(300, 0.5);
        } else {
            $("#colorPick").fadeTo(300, 1.0);
        };
    });

    $("#container #colorPick li a").click( function() {
        $("#container #colorPick li a").removeClass("active");
        $(this).addClass("active");
        var i = getIndex( $(this).parent() );
        var indexCandle = getIndex("#container #candlePick .active");
        $.globalEval("candle"+indexCandle+".color = '"+i+"';");
        $("#color").val(i); // nur für Einbau, Übergabe an Formular
        set();
    });

    setVisibleRange(); 
    center($("#candlePick"));
    center($("#colorPick"));
    set();

});

Answer 1

Your script is quite long, so I'll see how far I can get. Maybe I'll add more later.

• Your fiddle is lacking the images, so I'm not 100% sure I understand the "keep the color" feature.
• Your comments should be in English if you post the code for review like this.
• Unfortunately the more puzzling functions such as setVisibleRange have no comments.
• You are using many global variables. Instead you should have a "namespace object" or a closure function, in which you store all "global" data. You may even consider working with objects, so that you can have multiple instances of your item picker on the same page.
• You should never ever need to use eval (or $.globalEval). It's slow and more importantly in 99.9 per cent of cases completely unnecessary.
• Some of your function names are quite poorly chosen. E.g. correctIndex sounds like it actually changes the index. Something like formatIndex would be better. And do I have to say something about "set"?
• You are converting the indexes unnecessarily to and fro between string and number.
• Are you aware of the consequences of not having a JavaScript-less fallback? Some statistics say up to 5% of users block JavaScript. This is obviously a shop web site. Can the owner afford losing 5% of his customers?
• You have far too much business logic in your code. Stuff like the hard-coded number of items or "left out color" should have no place in your JavaScript code. Instead of generating the li items in your JavaScript, have them generated server-side. That way your script doesn't even need to know how many items there are, or which items have been left out (by the server-side logic), it just works with the lis that are there. And if you also add radio buttons to the lis server-side (which you can hide with the JavaScript) then you have a chance to build a selector that works even for the users without JavaScript.

That's it for now. If I have some time later, I'll come back and add some comments specific to the code.