current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

I have a list of categories (in this example I used String but in my project I have a model) which must be sorted like this:

  • on top we should have 2 categories which have a fixed position in the list (in this example (C, F))
  • at the bottom of the list we should have 3 categories which have a fixed position in the list (in this example (R, M, P))
  • in the middle of the list we should have the rest of the elements sorted alphabetically

I want to know if there is a better solution than I used, for sorting the list this way. I do not want to use HashMap, so if anyone could find a better solution for sorting the ArrayList, I would highly appreciate it.

List<String> categories = new ArrayList<>();
    categories.add("A");
    categories.add("B");
    categories.add("C");
    categories.add("D");
    categories.add("E");
    categories.add("F");
    categories.add("G");
    categories.add("H");
    categories.add("I");
    categories.add("J");
    categories.add("K");
    categories.add("L");
    categories.add("M");
    categories.add("N");
    categories.add("O");
    categories.add("P");
    categories.add("Q");
    categories.add("R");

    sortCategories(categories);

The sortCategories() method is: 

public void sortCategories(List<String> categories) {
    List<String> topCategories = new ArrayList<>();
    List<String> bottomCategories = new ArrayList<>();

    for (String category : categories) {
        if (category.equals("C")) {
            topCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("F")) {
            topCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("R")) {
            bottomCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("M")) {
            bottomCategories.add(category);
            break;
        }
    }

    for (String category : categories) {
        if (category.equals("P")) {
            bottomCategories.add(category);
            break;
        }
    }

    categories.removeAll(topCategories);
    categories.removeAll(bottomCategories);

    Collections.sort(categories, new Comparator<String>() {

        @Override
        public int compare(String lhs, String rhs) {
            return lhs.compareTo(rhs);
        }
    });

    categories.addAll(0, topCategories);
    categories.addAll(categories.size(), bottomCategories);
}

And the output is:

Category NAME C
Category NAME F
Category NAME A
Category NAME B
Category NAME D
Category NAME E
Category NAME G
Category NAME H
Category NAME I
Category NAME J
Category NAME K
Category NAME L
Category NAME N
Category NAME O
Category NAME Q
Category NAME R
Category NAME M
Category NAME P
share|improve this question

2 Answers 2

up vote 3 down vote accepted

You should not hard-code category values into the code, but rather make them configurable, so that your code may be re-used with other top/bottom category values. Since you have a specific sorting algorithm, it is probably best to create a custom comparator, which encapsulates all of the sorting logic. That comparator would be initialized with the values allowed for top/bottom categories, and would do the sorting, taking these values into account.

Using the comparator should be as simple as:

    List<String> allCategories = Arrays.asList(
            "A", "B", "C", "D", "E", "F",
            "G", "H", "I", "J", "K", "L",
            "M", "N", "O", "P", "Q", "R"
            );
    List<String> topCategories = Arrays.asList("C", "F");
    List<String> bottomCategories = Arrays.asList("M", "P", "R");
    Collections.sort(allCategories, new CategoryComparator(topCategories, bottomCategories));

There are many ways to implement such a comparator.The main idea is to extend the comparison logic: in addition to comparing values, you also compare their belonging to a particular category, and adjust the score consequently. As a result, values belonging to the top category should be put on top, and if two values belong to the same category, they should be compared according to their natural order.

public class CategoryComparator implements Comparator<String> {

    private static final int POSITIVE_SCORE = +1;
    private static final int NEGATIVE_SCORE = -1;
    private static final int TOP_SCORE_ADJUSTMENT = -2;
    private static final int BOTTOM_SCORE_ADJUSTMENT = +2;

    private Set<String> topCategories;
    private Set<String> bottomCategories;

    public CategoryComparator(Collection<String> topCategories, Collection<String> bottomCategories) {
        this.topCategories = new HashSet<>(topCategories);
        this.bottomCategories = new HashSet<>(bottomCategories);
    }

    @Override
    public int compare(String o1, String o2) {
        return normalizeScore(o1.compareTo(o2)) + ajustScoreForCategory(o1) - ajustScoreForCategory(o2);
    }

    private int normalizeScore(int score) {
        if (score < 0) {
            return NEGATIVE_SCORE;
        }
        if (score > 0) {
            return POSITIVE_SCORE;
        }
        return 0;
    }

    private int ajustScoreForCategory(String value) {
        int scoreAdjustment = 0;
        if (topCategories.contains(value)) {
            scoreAdjustment += TOP_SCORE_ADJUSTMENT;
        }
        if (bottomCategories.contains(value)) {
            scoreAdjustment += BOTTOM_SCORE_ADJUSTMENT;
        }
        return scoreAdjustment;
    }
}
share|improve this answer
up vote 3 down vote

Your code has two major flaws:

1) It has bad runtime behaviour:

First looking up the special cases, treat them remove them. treat the rest. This are too many iterations over the source material

2) It is unneccessary complex:

From (1)

Alternatively:

It depends on how much effort you want to put in, otherwise, this will do (for your simple case):

JAVA 8:

private List<String> sortCategories(List<String> allCategories) {
    String[] topCategories=new String[2];
    String[] bottomCategories=new String[3];
    SortedSet<String> rest = new TreeSet<>();
    List <String> result=new ArrayList<>();
    allCategories.stream().forEach(
            x->{
                switch(x){
                    case "C":
                        topCategories[0]=x;
                        break;
                    case "F":
                        topCategories[1]=x;
                        break;
                    case "M":
                        bottomCategories[0]=x;
                        break;
                    case "P":
                        bottomCategories[1]=x;
                        break;
                    case "R":
                        bottomCategories[2]=x;
                        break;
                    default:
                        rest.add(x);
                }
            }
    );
    result.addAll(Arrays.asList(topCategories));
    result.addAll(rest);
    result.addAll(Arrays.asList(bottomCategories));
    return result;
}

Pro:

  • Does, what it should.

  • Is simple.

  • Easy to understand.

  • Easy to maintain.

  • Easy to extend.

  • Runs linear through your source data set

Contra:

  • Hard codes the positions of the top / bottom categories

  • Uses a "nasty" switch

So, I would prefer this approach as a first shot. And if you have to later add more complexity: refactor it!

But perhaps YAGNI comes, and you have never to revisit this "ugly" code. There is no overcomplex Abstraction or Overengineering invested.

Of course, if you are dealing with objects instead of Strings you have to take care, that they are comparable to each other.

share|improve this answer
    
Thanks for your answer. Interesting approach. But unfortunately I work on Android (I forgot to mention, sorry) and it doesn't support Java 8. But it's good to know :) Thanks again. –  Laura Feb 8 at 8:16

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.