2
\$\begingroup\$

I'm building a wrapper for the Stackoverflow Careers RSS feeds for a NodeJS application and I've hit an interesting point.

The following code works fine and produces a valid result but I feel like I can handle it better.

Here's my Utils.js file:

let utils = {
    UrlPrefix: 'https://stackoverflow.com/jobs/feed?q=',
    Location: "&l=",
    Unit: "&u=",
    TechLiked: "&tl=",
    TechDisliked: "&td="
}

module.exports = utils;

And the function that I'm getting cold sweats from:

function UrlBuilder(props) {

    let builtUrl = utils.UrlPrefix;

    if(props.query) { 
        builtUrl += props.query
    }

    if(props.location) {
        builtUrl += utils.Location + props.location
    }

    if(props.td) {
        builtUrl += utils.TechDisliked + props.td
    }

    if(props.tl) {
        builtUrl += utils.TechLiked + props.tl
    }

    if(props.unit) {
        builtUrl += utils.Unit + props.unit
    }

    return builtUrl;
}

Is there no way around this or can I handle this better? All of the properties are optional so any one of them could be absent.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

It depends on what you want to make better. Personally, I find the code readable as you have it, however, depending on how many url options might still come, I guess you can make it yourself a bit easier.

A first start I would do, would be to split the query from the url itself, maybe you will not always add a query, so, I would change the utils class to:

let utils = {
    url: 'https://stackoverflow.com/jobs/feed',
    query: 'q=',
    location: "&l=",
    unit: "&u=",
    techLiked: "&tl=",
    techDisliked: "&td="
};

Furthermore, I also camelCased the properties of the utils object, I don't see no reason why they should be PascalCased. But I guess that could depend on some code guidelines you are following.

The same thing goes for the UrlBuilder. I would expect that PascalCased functions are classes, but since you return a string at the end of the function, it would not be the case. So you should also use camelCasing for simple functions. Together with that change, you could also introduce default values for your props parameter, so you wouldn't have to do checks for undefined/null values, like so:

/**
 * @method urlBuilder
 * @param {{query: string=, location: string=, td: string=, tl: string=, unit: string=}} props
 * @returns Full url needed to query rss feed
 */
function urlBuilder(props = {}) { ... }

If you expect a certain type of input parameters, it never hurts to document what does might be. It would help you further in the future when maintaining this code.

Now, the biggest difference I could imagine making is inside the urlBuilder function. One major change you could make is to choose for a more obvious name, urlBuilder itself sounds a bit like a wizard function to which you could add more options in a fluent language style. Maybe you could change it to getSpecificUrlFor(props = {}) which would indicate that you would receive a url based on the properties itself.

Now, inside the function, I added a small map that maps the expected properties of your argument to the properties in your utils object.

const map = {
  query: 'query',
  location: 'location',
  unit: 'unit',
  tl: 'techLiked',
  td: 'techDisliked'
};

The property of the map will indicate the expected property for your props object, and the value would link to the property on your utils object.

The biggest advantage you have here is that you can expand or reduce the parameters like you want to, if you would implement the check in the following way:

let result = [];
// iterate the full props object by it's key
for (const key of Object.keys( props )) {
  if (!map[key]) {
    // no matching prop found
    continue;
  }
  // found a key, so add it
  result.push(utils[map[key]]);
  result.push(props[key]);
}

Only if a property is defined on the map, it will add the utils value to your result set, together with the value of the props.

As the query is no longer part of the UrlPrefix property, you could now either return the url with potentially found parameters, or just return the default url, like so:

if (!result.length) {
  // no matching props
  return utils.url;
}
// return the url
return utils.url + '?' + result.join('');

There is no overly pragmatic reason for choosing the array.Join method, but rather a habit from using StringBuilder in other languages, so concatenating your strings as in your example is perfectly fine as well.

In the end, the full change looks like this, I added some example outputs as well so you could validate if it is now working as you expect it to work.

let utils = {
    url: 'https://stackoverflow.com/jobs/feed',
    query: 'q=',
    location: "&l=",
    unit: "&u=",
    techLiked: "&tl=",
    techDisliked: "&td="
};

/**
 * @method getSpecificUrlFor
 * @param {{query: string=, location: string=, td: string=, tl: string=, unit: string=}} props
 * @returns Full url needed to query rss feed
 */
function getSpecificUrlFor(props = {}) {
  const map = {
    query: 'query',
    location: 'location',
    unit: 'unit',
    tl: 'techLiked',
    td: 'techDisliked'
  };
  let result = [];
  // iterate the full props object by it's key
  for (const key of Object.keys( props )) {
    if (!map[key]) {
      // no matching prop found
      continue;
    }
    // found a key, so add it
    result.push(utils[map[key]]);
    result.push(props[key]);
  }
  if (!result.length) {
    // no matching props
    return utils.url;
  }
  // return the url
  return utils.url + '?' + result.join('');
}

// some valid inputs
console.log( getSpecificUrlFor({query: 'javascript'}) );
console.log( getSpecificUrlFor({query: 'javascript', tl: 'react'}) );
console.log( getSpecificUrlFor() ); // props would be at least an empty object, so expect the urlPrefix

// overwriting props that are not in the map doesn't work
console.log( getSpecificUrlFor({query: 'javascript', url: 'http://www.google.com'}) );

// invalid input -> gets ignored
console.log( getSpecificUrlFor(5) );
console.log( getSpecificUrlFor('a') );

One more thing you could still add, is to make sure that your parameters are properly encoded, you could do this with encodeURIComponent or something similar for nodejs

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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