How to avoid duplication of code when working with the DOM in the direction of "up" and "down"?

Question

I am writing a JS Webapp client. The user can edit the list / tree of text elements (for example, a list or notes). I manipulate the DOM a lot with jQuery.

The user can move up and down the list using the keyboard (similar to the J / K keys in GMail) and perform several other operations. Many of these operations have an up / down mirror function, for example.

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

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

QUESTION: How to edit this gracefully to avoid code duplication?

The trivial way I came with 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 advantage is simplified maintenance when the structure of other code elements requires a change to moveUp / moveDown. I already had to change the code a few times a few times, and I always need to keep in mind that I need to do this in two places ...

But I do not like the "unified" version, because:

• The implementation is more complicated than just moveUp / moveDown, so in the future it may not be so easy to maintain after. I like the simple code. All these parameters, triple operations, .apply () tricks in each function, which should go up and down ...
• Not sure if it is safe to refer to jQuery.fn functions. * explicitly (in the end they should be used by applying them to the jQuery $ ('...') object. *)

How do you solve these “almost identical code” situations when working with a DOM or similar structure?

Answer 1

One thing you can do is use closure to hide the configuration parameter passed 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 work in this direction, basically solve the question of how best to pass the parameters of the overall implementation, and there is no single best solution for this.

---

One of the possible directions is to use the OO style approach, passing the object of the strategy “strategy” 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 to do this when the strategy interface (methods that differ from each other) is small and relatively constant, and when you want to open up the possibility of adding new kinds of directions in the future.

I also use this when I know that neither the set of directions nor the methods should change much, since JS supports OO better than for switch statements.

---

Another possible direction is the enumeration approach 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]); 

Although I have to admit that this is rather clumsy, it has the advantage that if your set of directions is fixed at an early stage, you now have many options to add a new if-else or switch statement when you need it offline ( without having to add some methods to the strategy object somewhere else ...)

---

By the way, I think that the approach you proposed sits in an uncomfortable middle between the two versions that I proposed.

If everything you do switches inside your function depending on the value direction tag passed in, it’s best to just switch directly to the spots you need so as not to redo things into separate functions and then do a lot of annoying .call and .plply .

On the other hand, if you are faced with the problem of defining and separating functions, you can do it in such a way that you simply receive them directly from the caller (in the form of a strategy object) instead of manually scheduling them yourself.

Answer 2

I used to encounter similar problems, and there is not really a big template that I found to work with similar tasks in the reverse order.

While for you, as a person, it makes sense that they are similar, you really just call arbitrary methods on the compiler. The only obvious duplication is the order in which it is called:

• find the item we would like to insert before or after
• check if element exists
• insert the current item before or after it

You have long resolved a problem that basically performs these checks. Your decision, however, is a bit unreadable. Here is how I would solve it:

 $.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); } } 

Using the index of the array, we can see what is called when it is called a little easier than in your example. In addition, using the application is not required, since you know how many arguments you pass, so the call is better suited for this use case.

Not sure if this is better, but it's a little shorter (need a lie?) And a bit more readable IMO.

In my opinion, centralizing actions is somewhat more important than readability - if you want to optimize, change, or add something to the move action, you only need to do it in one place. You can always add comments to solve the problem of readability.

Answer 3

I think a single-function approach is the best, but I would separate it from this thing up / down. Make one function to move an element to the index or in front of another element (I would prefer the latter) and handle all the "moving" events in general there, regardless of 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); }