Javascript Function not Called onClick()

I was trying to clean up some of the code on our website (in its current form, we would have to make 50 or so JS files and 50 or so PHP files for all of our offered products). I am looking at the code and it SHOULD be working, so I am confused as to where I screwed up. It doesn't seem like the Function is being called at all (I put an Alert at the top of it that never got called) so I assume I screwed up something in the first section.

                <select class="gameserverselecion4" name="slots">
                    <option value="10|457">10 Slots - 9.90 / mo</option>
                    <option value="12|458">12 Slots - 11.88 / mo</option>
                    <option value="14|459">14 Slots - 13.86 / mo</option>
                <option value="16|460">16 Slots - 15.84 / mo</option>
                    <option value="18|461">18 Slots - 17.82 / mo</option>
                    <option value="20|462">20 Slots - 19.80 / mo</option>
                    <option value="22|463">22 Slots - 21.78 / mo</option>
                    <option value="24|464">24 Slots - 23.76 / mo</optio开发者_如何转开发n>
                    <option value="26|465">26 Slots - 25.74 / mo</option>
                    <option value="28|466">28 Slots - 27.72 / mo</option>
                    <option value="30|467">30 Slots - 29.70 / mo</option>
                    <option value="32468">32 Slots - 31.68 / mo</option>
            </select>
            </div>
            <div id="inputTopRight">
                <input type="button" name="submit" onclick="ProcessOrder('0','167','44');"/>
            </div>

Then the Javascript...

function ProcessOrder(loc,pid,cfopID)
{
    var values = document.dropTop.slots.value;
    var valuesArray = values.split("|");
    var slots = valuesArray[0];
    var cfopValue = valuesArray[1];
    alert("Slots is: " + slots " cfopValue is:" + cfopValue);

    switch (slots)
    {
        case 10:
        {
            window.location = "https://www.xfactorservers.com/clients/cart.php?a=add&pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue;
        }
        case 12:
        {
            window.location = "https://www.xfactorservers.com/clients/cart.php?a=add&pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue;
        }

Your code has several issues.

Syntax issues

1. It's better to avoid putting braces on their own lines, or automatic semicolon insertion will get you at some time.

Update (July 21, 2015): As pointed out in the comments, it should be fine in this case, as returning an object literal is different and an edge case. I still prefer to put the brace on the same line just to be on the safe side, though.

So instead of this:

function sayHello()
{
    alert('Hello, world!');
}

function sayHello() {
    alert('Hello, world!');
}

2. You shouldn't use braces around cases. Instead, end them with break statements.

Update (July 21, 2015): Using braces is fine, but they do not replace break. If you don't use it, it will "fall through" to the next case after that particular case is executed.

Wrong:

switch (number) {
    case 4: {
        doSomething();
    }
    case 9: {
        doSomething();
    }
}

Correct:

switch (number) {
    case 4:
        doSomething();
        break;
    case 9:
        doSomething();
        break;
}

3. The switch statement and the function isn't closed.

4. This line has a syntax error.

alert("Slots is: " + slots " cfopValue is:" + cfopValue);
//                         ^---here

This is the correct syntax:

alert("Slots is: " + slots + " cfopvalue is:" + cfopvalue);

Styling issues

1. You can combine multiple var statements.

Instead of this:

var a = 'foo';
var b = 'bar';
var c = 'baz';

You can do this:

var a = 'foo',
    b = 'bar',
    c = 'baz';

2. You can combine multiple cases in switch statements.

switch (number) {
    case 3:
    case 12:
        doSomething();
        break;
}

3.

alert("Slots is: " + slots + " cfopvalue is:" + cfopvalue);

This will show something like this:

Slots is: 10 cfopvalue is:20

You may want to insert a line break (\n) so that it looks better:

alert("slots is: " + slots + "\n" + "cfopvalue is: " + cfopvalue);

Slots is: 10
cfopvalue is: 20

4. Keep your content, styles and scripts separate. So instead of using inline event handlers, it's better to set up event handlers in your JS code, like this:

function addEvent(element, eventType, eventHandler) {
    if (window.addEventListener) {
        element.addEventListener(eventType, eventHandler, false);
    } else {
        element.attachEvent('on' + eventType, eventHandler);
    }
}

Example usage:

addEvent(document.getElementsByName('submit'), 'click', function() {
    ProcessOrder('0', '167', '44');
});

You can also use a library like jQuery ^[docs]. It will make things much simpler.

Update (July 21, 2015): Libraries like jQuery are a useful tool, but you should not rely entirely on them. It's important to be familiar with "vanilla" JavaScript too.

jQuery handles cross-browser incompatibilities for you, and uses CSS selectors to select elements. For reference, this is how you'd write the above event handler code with jQuery:

$('[name="submit"]').click(function () {
    ProcessOrder('0', '167', '44');
});

This line has a syntax error:

alert("Slots is: " + slots " cfopValue is:" + cfopValue);
//                         ^-- here

...which is probably why you're not seeing the alert. If you fix it, you may find that the function gets called (since the syntax error above prevents it getting defined).

Once you get it to the point the function is called, you still have an (unrelated) problem: You have the syntax of your switch statement wrong, although in fairly harmless way provided you really do want all of the conditions to fall through into each other. Individual cases are not enclosed in braces, those braces you have there are completely ignored by the parser. You terminate a case with break, so:

switch (slots)
{
    case 10:
        window.location = "https://www.xfactorservers.com/clients/cart.php?a=add&pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue;
        break;

    case 12:
        window.location = "https://www.xfactorservers.com/clients/cart.php?a=add&pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue;
        break;

    default:
        // do whatever you do if nothing matches (this is optional)
        break;
}

Without the breaks, the code for each case will flow into the next. So for instance, in your original, if slots were 10, first you'd set the window location to the one in the 10 case, then execution would continue with the 12 case and set it to a different location, etc., etc.

Off-topic: You can factor out a lot of commonality in that function; example:

function ProcessOrder(loc,pid,cfopID)
{
    var values = document.dropTop.slots.value;
    var valuesArray = values.split("|");
    var slots = valuesArray[0];
    var cfopValue = valuesArray[1];
    var destinationMap = {
            "10": "pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue,
            "12": "pid=" + pid + "&configoption[" + cfopID + "]=" + cfopValue,
            "14": "pid=167&configoption[44]=459",
            "16": "pid=167&configoption[44]=460",
            "18": "pid=167&configoption[44]=461",
            "20": "pid=167&configoption[44]=462",
            "22": "pid=167&configoption[44]=463",
            "24": "pid=167&configoption[44]=464",
            "26": "pid=167&configoption[44]=465",
            "28": "pid=167&configoption[44]=466",
            "30": "pid=167&configoption[44]=467",
            "32": "pid=167&configoption[44]=468"
        };
    var dest = destinationMap[slots];
    if (dest) {
        window.location = "https://www.xfactorservers.com/clients/cart.php?a=add&" + dest;
    }
    else {
        // Do whatever you should do if `slots` isn't a value you recognize
    }
}

(I left the pid= in each string because it seemed to me it helped with context.)