# Move object by one up or down algorithm in a custom order

Basically, I did an object (using hibernate) with a field called sorting_order. This field needs to be unique and I wish to swap two object by one. So one element has to be after or before the current element to be able to move. My object can only move by one up or down. I came up with this code, but I was wondering if there was a better way to do this.

Thank you!

/**
* Move two personal task macro
*
* @param userProfileModel - The {@link UserProfileModel}.
* @param move - true = up or false = down.
*/
private void movePersonnalTaskMacro(UserProfileModel userProfileModel, boolean move) {
Assert.notNull(userProfileModel, "userProfileModel required.");

logger.info("Preparing to move a personnal task macro.");

/* Initialize sorting order (increment/decrement) */

/* Condition validation */

if (move == false && selectedPersonnalTaskMacroIndex >= 1) {

logger.info("Moved the selected personnal task macro down by one");
}

logger.info("Moved the selected personnal task macro up by one");
}

}
}

-

private void movePersonnalTaskMacro (UserProfileModel userProfileModel, boolean move) {


This is a private method but it does not access any members of its class. It is a smell. This probably means its home is one of the parameters, (usually first). UserProfileModel seems like a case of smurf typing. boolean move we will probably need a Replace Parameter with Method

PersonnalTaskMacro selectedPersonnalTaskMacro = userProfileModel.getSelectedPersonnalTaskMacro();

/* Initialize sorting order (increment/decrement) */


A case of Inappropriate Intimacy. Call chains such as userProfileModel.getSelectedPersonnalTaskMacro().getSortingOrder() usually are.

int personnalTaskMacroListSize = userProfileModel.getPersonnalTaskMacroList().size();


More cases of the same. Also we see down .getPersonnalTaskMacroList() returns the actual field (Because you can sort it outside of the owner class). The collection should be encapsulated.

Since you plan to do nothing if personnalTaskMacroListSize is not greater than 1. say explicitly so. It makes it more readable, and saves you also from Arrow Code.

if (personnalTaskMacroListSize > 1) {


to

if (personnalTaskMacroListSize <= 1) return ;


Dispatch on parameter, is a bad idea:

  if (move == false

if (move == true


Moreover it is not needed in the first place. Many times user clicks Up or Down, and instead of

  onUpClicked() {
}

onDownClicked() {
}


We get something like:

  onUpClicked() {
}

onDownClicked() {
}


Less clear while reading the call site. Less clear while reading the implementation. Hides (some times loses) the users intent in the meantime.

personnalTaskMacro.getSortingOrder PersonnalTaskMacro.setSortingOrder Why does a Personal Task Macro needs to know in what order it participates in some collection of User Profile. If we decide to have a dozen more different lists of such macros, will we add a dozen more fields to Personal Task Macros. I am guessing this was because of some ORM issue. You had a sort order column in personalTaskMacro table so you had a property in that class. If that is the case you should fix your mapping. If not, why was it?

Collections.sort(userProfileModel.getPersonnalTaskMacroList());


Why do you need to sort if you only swapped two elements of the list. Also as mentioned above collections should be encapsulated. (If you need to have a property so your ORM can access it, make those getter and setter private.)

Here is my suggestion. Main refactorings are Move Method and Replace Parameter with Explicit Methods and Replace Nested Conditional with Guard Clauses. Also DRY the element swapping as suggested by @tomdemuyt.

class UserProfile {
int selected;

public void moveSelectedUp() {

if (selected <= 0) return;

selected--;
}

public void moveSelectedDown() {

if (selected >= personalTaskList.size() - 1)  return;

selected++;
}

static <T> void swap(List<T> list, int a, int b) {
T tmp = list.get(a);
list.set(a, list.get(b));
list.set(b, tmp);
}
}

-
Loved all the explanations and solution is quite elegant . I started out with a swap solution too, but was stuck because of ORM (as you said I have a field which contains my sorting order)... –  fneron Feb 14 '13 at 14:26

My thoughts in somewhat random order

• A boolean that indicates up/down could be improved with public 2 constants in your class UP = 0, DOWN = 1, and then the caller uses either constant. Or even better, use and ENUM as mentioned by cl-r.

• To be more DRY I would create 1 method that can swap any 2 people, then I would do something like

if (move == UP ) {
}
else {

Rather than using the two constants, it would be better to name the parameter moveUp, then just check it's value alone (rather than comparing it to true/false). However, as boolean parameters are to be avoided, that suggests something else should be done - perhaps some sort of moveUp(...)/moveDown(...) combo. –  Clockwork-Muse Feb 13 '13 at 22:32
You can also use enum direction {UP,DOWN} - swapPersonnal( move == direction.UP) ? .., .. -1 : .., .. +1) –  cl-r Feb 14 '13 at 10:25