2
\$\begingroup\$

I have a function that has, as input, a ranges object which may define from, to and limit. Note that it might have one, two or all three of them.

I want the output to be a saneRanges object, where if partial values are provided, the other ones are inferred.

For example if only limit is provided, from should be 0 and to should be limit-1.

This is the code I wrote during prototyping. I made sure I took zero shortcuts, as I didn't want smarty pants code make debugging harder.

But now... here I am. I have this lame code, and I want to find the bestest way to write it.

How would you rewrite this, so that it doesn't look quite so terrible?

  sanitizeRanges: function( ranges, skipHardLimitOnQueries ){
    var self = this

    var saneRanges = {};
    if( ! self.hardLimitOnQueries ) var hardLimitOnQueries = 0;

    saneRanges.from = 0;
    saneRanges.to = 0;
    saneRanges.limit = 0;

    if( typeof( ranges ) === 'object' && ranges !== null ){

      // Copy values over to saneRanges
      saneRanges.from = ranges.from || -1;
      saneRanges.to = ranges.to || -1;
      saneRanges.limit = ranges.limit || -1;

      var sr = saneRanges;

      // Sorry, no shortcuts here for now. Code will be optimised later
      // (maybe)

      // Case: Nothing is set
      if( sr.from === -1 && sr.to === -1 && sr.limit === -1 ){
         sr.from = 0;
         sr.to = self.hardLimitOnQueries;
         sr.limit = self.hardLimitOnQueries;

      // Case: Only "limit" is set
      // - Set "from" and "to"
      } else if( sr.from === -1 && sr.to === -1 && sr.limit !== -1 ){
        sr.from = 0;
        sr.to = sr.limit - 1;

      // Case: Only "from" is set
      // - Set "to" and "limit"
      } else if( sr.from !== -1 && sr.to === -1 && sr.limit === -1 ){
        sr.limit =  0;
        sr.to  = 0;

      // Case: Only "to" is set
      // - Set "from" and "limit"
      } else if( sr.from === -1 && sr.to !== -1 && sr.limit === -1 ){
        sr.from = 0;
        sr.limit =  sr.to + 1;

      // Case: Only "from" and "limit" are set
      // - Set "to"
      } else if( sr.from !== -1 && sr.to === -1 && sr.limit !== -1 ){
        sr.to =  sr.from + sr.limit - 1;

      // Case: Only "from" and "to" are set
      // - Set "limit"
      } else if( sr.from !== -1 && sr.to !== -1 && sr.limit === -1 ){
        sr.limit =  sr.to - sr.from + 1;

      // Case: Only "to" and "limit" are set
      // - Set "from"
      } else if( sr.from === -1 && sr.to !== -1 && sr.limit !== -1 ){
        sr.from = 0;
      }

      // Make sure "limit" never goes over
      if(  sr.limit != 0 && sr.from + sr.limit - 1 > sr.to ){
        sr.limit =  sr.to - sr.from + 1;
      }

    }

    // Apply hard limit on queries if required to do so. Driver implementations
    // should only pass 'true' for non-cursor queries, to prevent huge toArray() on
    // a million records
    if( ! skipHardLimitOnQueries ){
      if( self.hardLimitOnQueries && ( saneRanges.limit === 0 || sr.limit > self.hardLimitOnQueries ) ){
        saneRanges.limit = self.hardLimitOnQueries;
      }
    }

    return saneRanges;

  },
\$\endgroup\$
1
\$\begingroup\$

The code isn't especially bad and it seems to cover all bases but I agree it's not nice to look at. Perhaps some architectural observations may assist you in making it cleaner.

Firstly, I would not recommend using - 1 as a special value to indicate that a property is set or not. In this particular context it places an additional burden on you as the developer to remember what it means. Simply rely on the fact that a property is defined or it isn't.

A second observation is that I shouldn't worry about type checking unless you permit different "kinds" of range objects. If you've published your API that states a range object should be passed to this function, then have confidence that it will be used that way. If you must have it, consider inverting it so you don't have to nest the chain of if statements inside it.

A pattern I have seen regularly in this kind of code is to merge the user object with a default one, and then ask the question "does this make sense? ". With that said, be careful with modifying what has been passed to the function, unless behaviour is clearly published somewhere. What can happen is the user of this function will create a dodgy range object that gets corrected by your function and used in the DB query. The key part of this story is the user didn't know what they did was wrong and will carry on regardless. If it's wrong, give an error or let the query take ages and timeout, they'll have to learn some time!

\$\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.