2
\$\begingroup\$

I had some difficulties implementing a redis-like plugin basil.js, particularly the LREM method. The complexity was to loop only once (and not the whole array every time if we have 0 !== count) ascending or descending. My code below works and passes tests, but I was wondering if another smarter implementation (fewer intermediary vars) was possible.

Here is the whole source.

lrem: function (key, count, value) {
    if ('undefined' === typeof count || 'undefined' === typeof value)
        throw new Error('ERR wrong number of arguments for \'lrem\' command');

    var list = this.get(key) || [];

    if (0 === list.length)
        return 0;

    // added here for Basil, even if objects are supported
    // we to not support (yet?) object comparison, too heavy
    if ('number' !== typeof value && 'string' !== typeof value)
        throw new Error('ERR syntax error');

    var index = count >= 0 ? 0 : list.length - 1,
        length = list.length,
        iteration = 0,
        removed = 0;

    // iterate on whole list or stop if we found exactly the right count occurences
    while (iteration < length && (0 === count || (0 !== count && removed !== Math.abs(count)))) {

        if (value === list[index]) {
            list.splice(index, 1);
            removed++;

            // we removed an element, we need to decrease index if we are
            // looping from 0 to length otherwise we'll skip some values..
            if (count >= 0)
                index--;
        }

        iteration++;
        count >= 0 ? index++ : index--;
    }

    if (0 === list.length)
        this.remove(key);
    else
        this.set(key, list);

    return removed;
}
\$\endgroup\$
1
\$\begingroup\$

First some style notes: The "Yoda notation" is rare to see in JavaScript. Sure, it works, but you won't find many people using it, and just for that reason I'd recommend not using it, but it's your call. Just a heads-up in case others have to work on this code too.

I'd also recommend always using braces, even when they're not strictly necessary. Again, my recommendation, based on what I consider best practices.

In terms of functionality, it appears to match the docs for redis' LREM exactly - nice. Still I see some areas of improvement.

I don't see any need to avoid declaring local variables, though. You code seems just fine as-is on that point. Variables aren't a horrible performance drain or anything like that; there's no reason to avoid them. On the contrary, if you think adding a local variable will make the code more readable, then it's worth it.

Anyway, code review. First a simple thing: Use double quotes, and you don't have to escape single quotes in the string

throw new Error("Wrong number of arguments for 'lrem' command");

For that particular error, it's also easier to simply check arguments.length, and you could make it more informative - what is the correct number of arguments? (see below for an example of what I mean)

I've also removed the "ERR" prefix. It's already an Error object you're throwing, so it's kinda redundant.

Next, you can skip the || [] fallback, and instead do:

var list = this.get(key);

// if list is falsy or empty, or count is zero
if (!list || !list.length || !count) {
  return 0;
}

Next, another error. I've merely changed the wording a little, since it's not a syntax error; it's a semantic error. Besides, if it actually were a syntax error, you ought to be throwing a SyntaxError object. Here however, what we really want is a TypeError

if ('number' !== typeof value && 'string' !== typeof value) {
    throw new TypeError("'value' must be a string or a number");
}

But it might be simpler just to do all these checks upfront, instead of doing it in two different parts of the functions:

if(arguments.length < 3) {
  throw new Error("Wrong number of arguments for 'lrem' function (" + arguments.length + " for 3)");
}

if(typeof value !== 'number' && typeof value !== 'string') {
  throw new TypeError("'value' must be a string or a number");
}

And you might want to check the count argument's type too, while you're at it, but I'll leave that to you.

As for the actual meat of the function, I'd suggest simply using indexOf and lastIndexOf for simplicity.

var limit = Math.abs(count),
    removed = 0,
    indexOf = count < 0 ? list.lastIndexOf : list.indexOf,
    i;

while( removed < limit && (i = indexOf.call(list, value)) !== -1 ) {
  list.splice(i, 1);
  removed++;
}

if(!list.length) {
  this.remove(key);
}

return removed;

I've left out the this.set(key, list) line because I'm not sure it's necessary. While I'm not 100% sure (I just quickly skimmed the code on github), it seems that get returns a reference to the stored array, and, if so, you're removing items directly on that stored array already. No need to set it again.

Of course, if I'm wrong, just add the else-line back in.


All in one block:

lrem: function (key, count, value) {
  if(arguments.length < 3) {
    throw new Error("Wrong number of arguments for 'lrem' function (" + arguments.length + " for 3)");
  }

  if(typeof value !== 'number' && typeof value !== 'string') {
    throw new TypeError("'value' must be a string or a number");
  }

  // TODO: check 'count' type?

  var list = this.get(key);

  if (!list || !list.length || count == 0) {
    return 0;
  }

  var limit = Math.abs(count),
      removed = 0,
      indexOf = count < 0 ? list.lastIndexOf : list.indexOf,
      i;

  while( removed < limit && (i = indexOf.call(list, value)) !== -1 ) {
    list.splice(i, 1);
    removed++;
  }

  if(!list.length) {
    this.remove(key); // remove list entirely, if it's empty
  }

  return removed;
}
\$\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.