6
\$\begingroup\$

I am attempting to add a property to access the query string elements on the window and am looking for feedback on this implementation.

if (!window.location.hasOwnProperty("query"))
  Object.defineProperty(window.location, "query", {
      get(){
        var query = document.location.search.substr(1);
        if (query === undefined || query === "")
          return {};

        var pairs = query.split("&");

        var mapped = pairs.map(function(v) {
          var temp = v.split("=");
          return { key: temp[0], val: temp[1] };
        });

        var obj = mapped.reduce(function(total, current){
            total[current.key] = decodeURIComponent(current.val)
                                         .replace(/\+/g, " ");
            return total;
        }, {});

        return obj;
      }
  });

Then I could us it like this

var myVal = window.location.query.customerId
\$\endgroup\$
6
\$\begingroup\$

For the most part this looks good to me - your code is easily understood and well formatted. However, there is still room for improvement.

  1. Don't bother creating mapped, that logic could easily be integrated into the reduce function and avoids an extra loop over the array.

  2. Don't assign to an object just to return it, instead of doing var obj = ... return obj just do return pairs.reduce(...

  3. Parsing URLs is complicated. It's really easy to forget to encode / decode something when working with them. In this case, your code correctly decodes the query values, but fails to decode the query key - if my query is ?q%26=search, I would have to use document.location.query['q%26'] instead of the expected document.location.query['q&']. Is this likely to happen? Maybe not, but it should be handled.

    What I mean to say is - don't reinvent the wheel. There is a build in URLSearchParams object which does the parsing for you.

  4. What should happen if the URL is ?q=1&q=2? Should only the first value be kept? Only the last? What about ?q[]=1&q[]=2? Should this result in an array as PHP does or keep a single value? Ignoring these cases might be fine for your use case now, but if someone else uses your code, you should consider and document these edge cases.

Here's how I would implement this extension, ignoring the PHPism of converting keys using [] to arrays, and taking the last value if the same key is specified multiple times. I've defined it as a function for ease of testing, but it could just as well be dropped into your get() function.

function getQueryObject(query) {
  const result = {};
  const params = new URLSearchParams(query);
  for (const [key, value] of params) {
    result[key] = value;
  }
  return result;
}

;[
  '?q=1&q=2',
  '?q%26=URLUtils.searchParams&topic=api',
  '?q[]=a',
  '?q=with some spaces'
].forEach(query => {
  console.log(query, '-->', getQueryObject(query))
})

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Hey @Gerrit0, Thanks for the feed back. You raise a good point about the map call, no need to iterate over the collection more than once. As for #2 you are very correct but for the sake of readability I broke the chained functions out and stored them in variables however when this gets released everything after the first return of {} will all be chained on return command. You also raise an interesting oversight on my part with decoding the keys, I'm going to review/refactor that. #4 is the most interesting point for me, I think im going to refactor it to use the array. I'll post a revised copy \$\endgroup\$ – Adam H Jun 21 '18 at 14:20
3
\$\begingroup\$

I took the feedback from @Gerrit0 and have made the below changes to the method. One of the requirements was support for IE back to 8 (facepalm) so I couldn't implement URLSearchParams because we don't have a polyfill for that and was told to not implement it, yet we have a polyfill for map & reduce ... Anyways I also refactored a bit to store the parsed value instead of processing it on each request for the property. I also added array creation if a key is specified more than once while ignoring &test[]=2&test1[]=2 and just treating it as a key. I also threw it in an IIFE. Here is the revised snippet:

(function(window) {
  "use strict";

  if (!window.location.query) {
    window.location._query = {};

    var cleanVal = function(val) {
      return decodeURIComponent(val).replace(/\+/g, " ");
    };

    var query = window.location.search.substr(1);
    if (query) {
      window.location._query = query.split("&")
        .reduce(function(total, current) {
          var pair = current.split("=");
          var key = cleanVal(pair[0]);
          var val = cleanVal(pair[1]);

          if (total[key]) {
            if (total[key] instanceof Array.constructor) {
              total[key].push(cleanVal(val));
            } else {
              var tempVal = total[key];
              total[key] = [tempVal, val];
            }
          } else {
            total[key] = cleanVal(val);
          }

          return total;
        }, {});
    }

    try {
      Object.defineProperty(window.location, "query", {
        get: function() {
          return window.location._query;
        }
      });
    } catch (e) {
      // stupid IE support
      window.location.query = window.location._query;
    }

  }
}(window));

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ FWIW I kinda liked the way you did it before, chaining the different functions together, split, map, reduce. That was easier for me to see the overall flow the first read; I would have just moved the clean up to the mapping section so that reduce just reduces clean values. To my eye, I like the simpler, single-purpose functions and sequential processing... but people do it both ways, obviously. \$\endgroup\$ – ndp Jun 22 '18 at 3:49
  • \$\begingroup\$ @ndp, you raise a valid point here. Do I sacrifice a tiny bit of of performance for readability? Honestly now that I've had time to reflect on this method a bit I just might go back to the previous style while still keeping the feedback from Gerrit0. \$\endgroup\$ – Adam H Jun 22 '18 at 14:14
1
\$\begingroup\$

Looking good. One other case: no value. What do you want out of /path?value=1&paisley I usually expect: { value: 1, paisley: true }. You'll need to decide what you want out of (1) no value (2) empty value. I usually map these to booleans, but it depends on the semantics of your project.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Hey @ndp, thanks for the feedback and pointing out an obvious issue i've managed to overlook. I'll make sure to update that. \$\endgroup\$ – Adam H Jun 22 '18 at 14:12

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.