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.");

PersonnalTaskMacro selectedPersonnalTaskMacro = userProfileModel.getSelectedPersonnalTaskMacro();

/ Initialize sorting order (increment/decrement) /
int selectedPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacro.getSortingOrder();
int nextPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacroSortOrder + 1;
int previousPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacroSortOrder - 1;

/ Condition validation /
int personnalTaskMacroListSize = userProfileModel.getPersonnalTaskMacroList().size();
int selectedPersonnalTaskMacroIndex = userProfileModel.getPersonnalTaskMacroList().indexOf(
selectedPersonnalTaskMacro);

if (personnalTaskMacroListSize > 1) {
if (move == false && selectedPersonnalTaskMacroIndex >= 1) {
int previousPersonnalTaskMacroIndex = selectedPersonnalTaskMacroIndex - 1;
PersonnalTaskMacro previousPersonnalTaskMacro = userProfileModel.getPersonnalTaskMacroList().get(
previousPersonnalTaskMacroIndex);

previousPersonnalTaskMacro.setSortingOrder(selectedPersonnalTaskMacroSortOrder);
selectedPerson

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) */
int selectedPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacro.getSortingOrder();

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

int personnalTaskMacroListSize = userProfileModel.getPersonnalTaskMacroList().size();
int selectedPersonnalTaskMacroIndex = userProfileModel.getPersonnalTaskMacroList().indexOf(
        selectedPersonnalTaskMacro);

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() {
    movePersonnalTaskMacroUp();
  }

  onDownClicked() {
    movePersonnalTaskMacroDown();
  }

We get something like:

onUpClicked() {
    movePersonnalTaskMacro(true);
  }

  onDownClicked() {
    movePersonnalTaskMacro(false);
  }

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 {
    List personalTaskList;
    int selected;

    public void moveSelectedUp() {
        if (personalTaskList.size() = personalTaskList.size() - 1)  return;

        swap(personalTaskList, selected, selected + 1);
        selected++;
    }

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

StackExchange Code Review Q#21672, answer score: 5