How to avoid duplicate code when operating on DOM in "up" and "down" direction?

Question

I'm writing a JS webapp client. User can edit list/tree of text items (say, a todo list or notes). I manipulate DOM with jQuery a lot.

User can navigate the list up and down using keyboard (similar to J/K keys in GMail), and perform several other operations. Many of these operations have mirror "up" / "down" functionality, e.g.

$.fn.moveItemUp = function() {
    var prev = this.getPreviousItem();
    prev && this.insertBefore(prev);
    // there's a bit more code in here, but the idea is pretty simple,
    // i.e. move the item up if there's a previous item in the list
}

$.fn.moveItemDown = function() {
    var next = this.getNextItem();
    next && this.insertAfter(next);
    // ....
}

Now this pattern of having two almost identical functions is repeated in several places in my code because there are many operations on the list/tree of items that are pretty much symmetrical.

QUESTION: How do I refactor this elegantly to avoid code duplication?

The trivial way I came up with so far is to use .apply()...

$.fn.moveItem = function(direction) {
        var up = direction === 'up',
            sibling = up ? this.getPreviousItem() : this.getNextItem(), 
            func = up ? $.fn.insertBefore : $.fn.insertAfter;

        // ...

        if (! sibling) { return false; }

        func.apply(this, [ sibling ]);

        // ...
    };

The benefit is easier maintenance when structure of other elements of the code requires changing moveUp/moveDown. I've already needed to slightly change the code multiple times, and I always need to keep in mind that I need to do it in two places...

But I'm not happy with the "unified" version, because:

• Implementation is more complicated than simple moveUp/moveDown, hence in the future it may NOT be so easy to maintain afterall. I like simple code. All these parameters, ternary operations, .apply() tricks in every function that's supposed to go "up" and "down"...
• Not sure if it's safe to refer to jQuery.fn.* functions explicitly (after all they're supposed to be used by applying them to jQuery object $('...').*)

How do you solve those "almost identical code" situations when working with DOM or similar structure?

Answer 1

One thing that you can do is to use closures to hide the configuration parameter passing from the user:

var mk_mover = function(direction){
     return function(){
         //stuff
     };
};

var move_up = mk_mover("up");
var move_down = mk_mover("down");

If you want to continue in this direction, it becomes basically a matter of deciding what is the best way to pass the parameters to the common implementation, and there is no single best solution for this.

---

One possible direction to go is to use an OO style approach, passing a configuration "strategy" object to the implementation function.

var mk_mover = function(args){
    var get_item = args.getItem,
        ins_next = args.insNext;
    return function(){
        var next = get_item.call(this);
        next && ins_next.call(this, next);
    };
 };

 var move_up = mk_mover({
     get_item : function(){ return this.getPrevious(); },
     get_next : function(x){ return this.insertBefore(x); }
 });

 var move_down = mk_mover({/**/});

I prefer doing this when the strategy interface (the methods that differ between directions) is small and relatively constant and when you want to open up the possibility of adding new kinds of direction in the future.

I also tend to use this when I know that neither the set of directions nor the kinds of methods will need to change much, since JS has better support for OO then for switch statements.

---

The other possible direction to go is the enum approach that you used:

var mk_mover = function(direction){
    return function(){
        var next = (
            direction === 'up' ? this.getNextItem() :
            direction === 'down' ? this.getPreviousItem() :
            null );

        if(next){
            if(direction === 'up'){
                this.insertAfter(next);
            }else if(direction === 'down'){
                this.insertBefore(next);
            }
        }

        //...
    };
}

Sometimes you can use neat objects or arrays instead of switch statements to make things prettier:

var something = ({
    up : 17,
    down: 42
}[direction]);

While I have to confess that this is kind of clumsy, it has the advantage that if your set of directions is fixed early on, you now have lots of flexibility to add a new if-else or switch statement whenever you need to, in a self-contained manner (without needing to go add some methods to the strategy object somewhere else...)

---

BTW, I think that the approach you suggested sits in an uncomfortable middle ground between the two versions I suggested.

If all you are doing are switch inside your function depending on the value direction tag passed, its better to just do the switches directly in the spots you need to without having to refactor things into separate functions and then having to do lots of annoying .call and .apply stuff.

On the other hand, if you went through the trouble of defining and separating the functions you can make it so that you just receive them directly from the caller (in the form of a strategy object) instead of doing the dispatching by hand yourself.

Answer 2

I've run into similar issues before, and there isn't really a great pattern I've found for dealing with doing seemingly similar tasks in reverse.

While it makes sense to you as a human that they're similar, you really are just calling arbitrary methods as far as the compiler is concerned. The only obvious duplication is the order in which things are called:

1. find an item that we'd like to insert before or after
2. check that the item exists
3. insert the current item before or after it

You've solved the problem in one way already that basically does these checks. Your solution is however a little unreadable. Here's how I would solve that:

$.fn.moveItem = function(dir){
  var index = dir == 'up' ? 0:1,
      item = this['getPreviousItem','getNextItem'][index]();
  if(item.length){
    this['insertBefore','insertAfter'][index].call(this,item);
  }
}

By using an array index, we can see what is being called when it's being called a little easier than in your example. Also, the use of apply is unnecessary, as you know how many arguments you're passing, so call is better suited to that use case.

Not sure if that's any better, but it's a little shorter (is return false necessary?) and a bit more readable IMO.

In my opinion, centralizing actions is slightly more important than readability - if you want to optimize, change, or append to the action of moving something, you only have to do it in once place. You can always add comments to solve the readability issue.

Answer 3

I think the one-function approach is the best, yet I would detach it from that up/down thing. Make one function to move an item to an index or before an other item (I'd prefer the latter one) and handle all the "move" events generically there, not depending on a direction.

function moveBefore(node) {
    this.parentNode.insertBefore(this, node); // node may be null, then it equals append()

    // do everything else you need to handle when moving items
}
function moveUp() {
    var prev = this.previousSibling;
    return !!prev && moveBefore.call(this, prev);
}
function moveDown() {
    var next = this.nextSibling;
    return !!next && moveBefore.call(this, next.nextSibling);
}