using indexof on query string parameter with multiple values

Question

I'm showing hidden divs based on the contents of a query string parameter that's being parsed from another page, using indexof to check if a value is present - then displaying that value's corresponding div.

Query string: index.html?q1=bag1,bag2,bag3

    var urlParams;
    (window.onpopstate = function () {
        var match,
            pl     = /\+/g,  // Regex for replacing addition symbol with a space
            search = /([^&=]+)=?([^&]*)/g,
            decode = function (s) { return decodeURIComponent(s.replace(pl, " ")); },
            query  = window.location.search.substring(1);

        urlParams = {};
        while (match = search.exec(query))
           urlParams[decode(match[1])] = decode(match[2]);
    })();

Then using indexOf to show divs based on the value:

    if ((urlParams["q1"]).indexOf("bag1") >= 0) {
        $(".content1").show();

    } else

    if ((urlParams["q1"]).indexOf("bag2") >= 0) {
        $(".content2").show();

    } else

    if ((urlParams["q1"]).indexOf("bag3") >= 0) {
        $(".content3").show();

    }

However, it's only displaying the first div and not the second or third.

I know it'll be a simple solution - bit stuck on it. Any help appreciated!

possible duplicate of [How can I get query string values in JavaScript?](http://stackoverflow.com/questions/901115/how-can-i-get-query-string-values-in-javascript) — NDM, Sep 17 '15 at 10:15

score 3 · Answer 1 · answered Sep 17 '15 at 10:19

3

You need to remove the else clauses, as the interpreter will stop after the first if is true. So your code should look like

    if ((urlParams["q1"]).indexOf("bag1") >= 0) {
        $(".content1").show();

    }

    if ((urlParams["q1"]).indexOf("bag2") >= 0) {
        $(".content2").show();

    }

    if ((urlParams["q1"]).indexOf("bag3") >= 0) {
        $(".content3").show();

    }

answered Sep 17 '15 at 10:19

metal03326

1,213
2
13
15

this is technically correct, but I wouldn't accept it in a code review. – Alnitak Sep 17 '15 at 10:43
because it needlessly (and unnecessarily) repeats code over and over instead of parameterising the operation. It's also error prone (as the OP found) – Alnitak Sep 17 '15 at 19:20

score 2 · Accepted Answer · answered Sep 17 '15 at 10:49

I'd suggest using your bag1 etc values as IDs instead of classes to identify the separate content sections. If a class only identifies one element then you're doing it wrong.

You should then tag all of your content elements with the same class (e.g. content) so that you can .hide() on all of them before running .show on the ones that you want to remain visible. As written any elements that are already visible will remain visible when you pop state even if they weren't supposed to be.

Your parameter extracting code is OK, but having got your q1 value I would just do this:

var q1 = urlParams.q1;
if (q1 !== undefined) {
    $('.content').hide();    // show nothing
    q1.split(',').forEach(function(id) {
        $('#' + id).show();
    });
}

thereby removing all of the (broken) conditional logic.

using indexof on query string parameter with multiple values

2 Answers2