Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
                selectedPersonnalTaskMacro.setSortingOrder(previousPersonnalTaskMacroSortOrder);

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

            if (move == true && nextPersonnalTaskMacroSortOrder <= personnalTaskMacroListSize) {
                int nextPersonnalTaskMacroIndex = selectedPersonnalTaskMacroIndex + 1;
                PersonnalTaskMacro nextPersonnalTaskMacro = userProfileModel.getPersonnalTaskMacroList().get(
                        nextPersonnalTaskMacroIndex);

                nextPersonnalTaskMacro.setSortingOrder(selectedPersonnalTaskMacroSortOrder);
                selectedPersonnalTaskMacro.setSortingOrder(nextPersonnalTaskMacroSortOrder);

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

            Collections.sort(userProfileModel.getPersonnalTaskMacroList());
        }
    }
share|improve this question
add comment

2 Answers

up vote 5 down vote accepted
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<PersonalTask> personalTaskList;
    int selected;

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

        if (selected <= 0) return;

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

    public void moveSelectedDown() {
        if (personalTaskList.size() <= 1) return;

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

        swap(personalTaskList, selected, selected + 1);
        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);
    }
}
share|improve this answer
    
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
add comment

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 ) {
       swapPersonnal( selectedPersonnalTaskMacroIndex , selectedPersonnalTaskMacroIndex - 1 )
    }
    else {
      swapPersonnal( selectedPersonnalTaskMacroIndex , selectedPersonnalTaskMacroIndex + 1 )
    }
    
  • In general I find this code to be confusing, the data-model is not clear, this requires more comments, I cannot easily distinguish between

    • .getSortingOrder() giving selectedPersonnalTaskMacroSortOrder, I think I understand. This method give the current order of the objectin the database.
    • getPersonnalTaskMacroList().size(); , why do you need it, it is not obvious. I need it to don't get outofbound exception of my collection size. For instance, when I try to move up an object where there's no next object.
    • selectedPersonnalTaskMacroIndex, what is the difference with sortingOrder() ? Not obvious. The selectedPersonnalTaskMacroIndex represent the index in my java collection and the sortingOrder is a number in my database which is unique and acts as the custom sorting order.
    • userProfileModel.getPersonnalTaskMacroList().get( previousPersonnalTaskMacroIndex); How is the userProfile task macro list tied to the previous personnel macro index, at this point the code does not make any sense to me? This actually get the object to swap his order (aka setSortingOrder) with the current one. In this case, it's the previous one.
  • Personnal is not correctly spelled, not sure if you can still fix this. I should fix this lol.

share|improve this answer
1  
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.