3
\$\begingroup\$

I'm trying to optimize this repetitive piece of code. I think I need to set subsection href as a variable and then figure out a way to merge the functions?

$(document).ready(function () {
    $('#alphabetical').click(function () {
        $('.subsection').attr('href', function () {
            return this.href + '&sort=alphabetical';
        });
    });
    $('#chronological').click(function () {
        $('.subsection').attr('href', function () {
            return this.href.replace('&sort=alphabetical', '');
        });
    });
    $('#builtfilter').click(function () {
        $('.subsection').attr('href', function () {
            return this.href + '&status=built';
        });
    });
    $('#allfilter').click(function () {
        $('.subsection').attr('href', function () {
            return this.href.replace('&status=built', '');
        });
    });
});
\$\endgroup\$
4
\$\begingroup\$

First off, you have a couple of bugs:

You can keep clicking the #alphabetical or #builtfilter buttons, and each time you do, it'll add the parameter. Even if it's already there. Yet the "reverse" buttons only replace/remove 1 instance. So if you have:

 /some/path?foo=bar

and click the alphabetical button again a couple of times, you get

 /some/path?foo=bar&sort=alphabetical&sort=alphabetical&sort=alphabetical

But you'll have to click #chronological an equal number of times, to "reset" everything, since only one &sort=alphabetical chunk is removed each time.

Secondly, I don't know the URL you're manipulating, but if there are no parameters to begin with, you end up with an invalid URL like:

/some/path&sort=alphabetical

Note that there's no ? to separate the query string from the path. Instead the & has ended up there - where it shouldn't be.

Also, if you URL has a fragment like #something, you'll be adding paramters after that, which won't work right. Like:

/some/path?foo=bar#fragment&sort=alphabetical

So first off, I'd recommend using jQuery's param method to create a well-formatted query string from an object:

var params = { sort: "", status: "built" }; // an example
var queryString = jQuery.param(params); //=> "sort=&status=built"

Secondly, you can use data-* attributes on your links to specify their action and classes to allow you to find all the links/buttons in one go (here's a related answer of mine).

For instance (simple example):

<button type="button" class="param" data-key="sort" data-value="alphabetical">Sort alphabetically</button>
<button type="button" class="param" data-key="sort" data-value="">Sort chronologically</button>
$(function () { // equivalent to $(document).ready(...)
  var params = { sort: "", status: "" }; // default params

  $(".param").on("click", function () {
    var $this = $(this),
        key = $this.data("param"),
        value = $this.data("value"),
        queryString;

    params[key] = value;
    queryString = jQuery.param(params);

    $(".subsection").attr("href", function () {
      // add or replace the query string as needed
      return this.href.replace(/(\?.*)?$/, "?" + queryString);
    });
  });
});

Note that this doesn't preserve fragments or any parameters that were already in the href, but not in the params object - but I'll leave that as an exercise.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Unfortunately I need it to preserve parameters since all the links have an id parameter by default but this is a helpful start. I assume I just need to figure out how to modify the replace value? Much appreciated. \$\endgroup\$ – sneep Aug 26 '15 at 17:26
  • \$\begingroup\$ @sneep Yeah, if you've already got some parameters, you need to do a little more work. I'd look at ways of parsing existing params, or at least detecting whether there are already params in the url (e.g. /\?.*$/.test(href)). You can also go a completely different route: Add a click listener to the .subsection links, and only change their urls right as they're clicked. E.g. get the sort and status params when the link is clicked, rather than updating all the links whenever something changes \$\endgroup\$ – Flambino Aug 26 '15 at 19:26
1
\$\begingroup\$

Changes to your code:

  • Enclosed it in an Immediately-Invoked Function Expression and "imported" the jQuery library.
  • Added 'use strict' inside the IIFE.
  • Used constants to denote magic values.
  • Replaced $(document).ready() with jQuery's short hand version.
  • Put everything inside one click event handler function.

The rest of it is pretty self-explanatory, I think. The code is a lot more verbose, but it makes everything a lot more flexible and DRY.

(function ($) {
    'use strict';

    var SUBSECTION_SLCTR    = '.subsection',
        ALPHABETICAL_SLCTR  = '#alphabetical',
        CHRONOLOGICAL_SLCTR = '#chronological',
        BUILTFILTER_SLCTR   = '#builtfilter',
        ALLFILTER_SLCTR     = '#allfilter';

    $(function () {
        var filterOptionSelectors = [
            ALPHABETICAL_SLCTR,
            CHRONOLOGICAL_SLCTR,
            BUILTFILTER_SLCTR,
            ALLFILTER_SLCTR
        ];

        $(filterOptionSelectors.join(', ')).click(function () {
            $(SUBSECTION_SLCTR).attr('href', function (i, value) {
                var $this = $(this);

                if ($this.is(ALPHABETICAL_SLCTR)) {
                    return value + '&sort=alphabetical';
                }

                if ($this.is(CHRONOLOGICAL_SLCTR)) {
                    return value.replace('&sort=alphabetical', '');
                }

                if ($this.is(BUILTFILTER_SLCTR)) {
                    return value + '&status=built';
                }

                // Remaining element is "all filter".
                return value.replace('&status=built', '');
            });
        });
    });
}(jQuery));

Note: Haven't had time to test this for errors.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.