Optimizing a Sort Method

Question

I have a program that needs to sort matches into the following order (the following is a short example):

PIT  
PREDICTIONS  
Match 1  
Match 2  
Quarters 1 Match 1  
Quarters 1 Match 2  
Quarters 2 Match 1  
Semis 1  
Semis 2  
Finals 1  
Finals 2

Note: There can be unlimited Matches, Quarters 1 - 4 only (with unlimited submatches), unlimited semis, and unlimited finals.

I've got the following method that assign a score to one of the above titles:

public static long getMatchScore(String name) {
    long score = 0;
    String matchName = name.toLowerCase();
    String[] tokens = matchName.split("\\s+");

    if(matchName.startsWith("pit")) score -= 100000;
    else if(matchName.startsWith("predictions")) score -= 1000;
    else if(matchName.startsWith("quals")) score = Integer.parseInt(matchName.split("\\s+")[1]);
    else if(matchName.startsWith("quarters")) {
        if(Integer.parseInt(tokens[1]) == 1) score += 1000;
        else if(Integer.parseInt(tokens[1]) == 2) score += 10000;
        else if(Integer.parseInt(tokens[1]) == 3) score += 100000;
        else if(Integer.parseInt(tokens[1]) == 4) score += 1000000;

        score += Integer.parseInt(tokens[3]);
    }
    else if(matchName.startsWith("semis")) {
        if(Integer.parseInt(tokens[1]) == 1) score += 10000000;
        else if(Integer.parseInt(tokens[1]) == 2) score += 100000000;

        score += Integer.parseInt(tokens[3]);
    }
    else if(matchName.startsWith("finals")) {
        score += 1000000000; 
        score += Integer.parseInt(tokens[1]);
    }
    return score;
}

Then the Java method compareTo() is used to sort it. Is there a better way to do this rather than assigning massive numbers like 100000000.

Answer 1

First of all, and I don't say this sarcastically, if you're going to write code like getMatchScore, you should be spending more time learning how to write clean code, than actually whipping out more code. The cyclomatic complexity of that method is thorugh the roof.

That said, if we recognize that your input has groups that have inherent weightage, like Match < Quarters, we should capture that in code using an Enum. A simple one with integer weightage should suffice, don't need a complicated one as shown by "Java Guy next door". How about the following code that I've tested to be working?

private static final Comparator<String> BY_GROUP = (s1, s2) -> {
    Map.Entry<Integer, Integer> e1 = toTuple(s1);
    Map.Entry<Integer, Integer> e2 = toTuple(s2);

    Comparator<Map.Entry<Integer, Integer>> c = Comparator.<Map.Entry<Integer, Integer>, Integer>comparing(Map.Entry::getKey)
            .thenComparing(Map.Entry::getValue);

    return c.compare(e1, e2);
};

private static Map.Entry<Integer, Integer> toTuple(String s) {
    String[] tokens = s.toUpperCase().split("\\s");
    Group group = Group.valueOf(tokens[0]);
    int num = tokens.length > 1 ? Integer.valueOf(tokens[1]) : Integer.MIN_VALUE;

    return new AbstractMap.SimpleImmutableEntry<>(group.weightage(), num);
}

private enum Group {
    PIT(0), PREDICTIONS(1), MATCH(2), QUARTERS(3), SEMIS(4), FINALS(5);

    private int weightage;

    Group(int weightage) {
        this.weightage = weightage;
    }

    public int weightage() {
        return this.weightage;
    }
}

public static void main(String[] args) {
    List<String> input = Arrays.asList("PIT", "PREDICTIONS", "Match 1", "Match 2",
            "Quarters 1 Match 1", "Quarters 1 Match 2", "Quarters 2 Match 1", "Semis 1", "Semis 2",
            "Finals 1", "Finals 2");

    input.sort(BY_GROUP);
    System.out.println(input);
}

Answer 2

This one calls for embedding the logic in your code into a Comparable class.

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class MatchDescription implements Comparable<MatchDescription> {

    private MatchType matchType;
    private int matchOrder = 0;
    private int subMatchOrder = 0;

    public MatchDescription(String name) {
        String matchName = name.toLowerCase().trim();
        String[] tokens = matchName.split("\\s+");

        matchType = MatchType.getByName(tokens[0]);

        if (matchType.hasMatchOrder()) {
            matchOrder = Integer.parseInt(tokens[1]);
        }
        if (matchType.hasSubmatches()) {
            subMatchOrder = Integer.parseInt(tokens[3]);
        }
    }

Now, methods compareTo(T) and equals(Object) are overridden using the logic of the matches order, sub-order etc.

    @Override
    public int compareTo(MatchDescription other) {
        if (this.matchType == other.matchType)
            if (this.matchOrder == other.matchOrder)
                return (this.subMatchOrder - other.subMatchOrder);
            else
                return (this.matchOrder - other.matchOrder);
        else
            return (this.matchType.getMatchTypeOrder() - other.matchType.getMatchTypeOrder());  
    }

    @Override
    public boolean equals(Object other) {
        return (other instanceof MatchDescription) &&
                (((MatchDescription) other).matchType == this.matchType) &&
                (((MatchDescription) other).matchOrder == this.matchOrder);
    }

The match types are managed via an inner enum:

    private enum MatchType {
        PIT("PIT",                 1, false, false),
        PREDICTIONS("PREDICTIONS", 2, false, false),
        MATCH("Match",             3, true,  false),
        QUARTERS("Quarters",       4, true,  true),
        SEMIS("Semis",             5, true,  true),
        FINALS("Finals",           6, true,  false)
        ;

        private String name;
        private int matchTypeOrder;
        private boolean hasMatchOrder;
        private boolean hasSubmatches;

        MatchType(String name, int matchTypeOrder, boolean hasMatchOrder, boolean hasSubmatches) {
            this.name           = name;
            this.matchTypeOrder = matchTypeOrder;
            this.hasMatchOrder  = hasMatchOrder;
            this.hasSubmatches  = hasSubmatches;
        }

        public boolean hasMatchOrder() {
            return hasMatchOrder;
        }

        public boolean hasSubmatches() {
            return hasSubmatches;
        }

        public static MatchType getByName(String matchName) {
            for (MatchType value : values()) {
                if (value.getName().equalsIgnoreCase(matchName))
                    return value;
            }
            return null;
        }

        private String getName() {          
            return name;
        }

        public int getMatchTypeOrder() {            
            return matchTypeOrder;
        }
    }

Finally, toString() presents the object nicely:

public String toString() {
        String description = matchType.getName();
        if (matchType.hasMatchOrder()) {
            description +=  " " + matchOrder;

            if (matchType.hasSubmatches()) 
                description +=  " Match " + subMatchOrder;
        }

        return description;
    }

This is an example on how to use the comparable object (notice that white spaces are also handled):

    public static void main(String[] args) {
        String[] inputs = new String[] {
                "PIT  ",
                "       Finals 1  ",
                "PREDICTIONS  ",
            "   Match 2  ",
            "   Quarters 1 Match 1  ",
            "   Quarters 1 Match 2  ",          
            "   Match 1  ",
        "       Semis 1  Match 1 ",
        "       Semis 2   Match 1",
        "       Finals 2",
        "   Quarters 2 Match 1  "};

        List<MatchDescription> c = new ArrayList<>();
        for (String input : inputs) {
            c.add(new MatchDescription(input));
        }
        Collections.sort(c);

        for (MatchDescription e : c) {
            System.out.println(e);
        }

    }
}

Answer 3

The first thing: the given code does contain duplicated code. Which is a bad thing by itself. But also a small performance hit.

Example: you are calling parseInt(token[0]) like 5 times or more. You could restructure the whole method to make it A) easier to read and B) compute that expression only once for example.