6
\$\begingroup\$

I recently wrote code that would query a collection of objects and rank them according to a given criteria and then filter based on an object property. I attach the code below. I would like to know if my approach can be improved upon such as made more efficient and extensible. Is there a better approach or just another approach?

I know that lambdas in Java 8 would make the code much cleaner. What other improvements are available?

Here's the scenario:

Rank journals numerically by score, those with a shared rank ordered lexicographically and filter out journals that are review journals.

Here's the data:

Given the following journals have scores:

Journal A = 2.2 
Journal B = 6.2
Journal C = 6.2
Journal D = 1.2

And Journal D is a review journal.

The result should be:

Rank  Journal Name    Score

 1    Journal B        6.2  
 1    Journal C        6.2  
 3    Journal A        2.2

Note: Journal D is filtered out of the list.

Here's my code:

This code produces the actual sort and filter:

List<Journal> journals = // add journals to collection.
Collate<Journal> collateJournals = new Collate<>();
Collate<Journal> collatedJournals = collateJournals.from(journals).filter(new OutReview()).sort(new ByScoreThenName(Direction.ASC)).rank(new Numerical());

This code controls the collation of sorted and filtered data.

public class Collate<T> {

    List<T> collection = new ArrayList<>();

    public Collate<T> sort(Comparator<T> sortComparator) {
        Collections.sort(collection, sortComparator);
        return this;
    }    
    public T get(int i) {
        return this.collection.get(i);
    }       
    public boolean contains(T element){
        return collection.contains(element);
    }    
    public Collate<T> from(List<T> collection) {
        this.collection = collection;
        return this;
    }   
    public Collate<T> filter(Predicate<T> predicate) {            
        List<T> result = new ArrayList<T>();
        for (T element : (Collection<T>) collection) {
          if (predicate.apply(element)) {
            result.add(element);
          }
        }   
        this.collection = result;
        return this;
    }

    public Collate<T> rank(Rank<T> rankEngine) {
        rankEngine.doRank(collection);      
        return this;
    }   
}

This code implements a Comparator interface and is used to sort the collection: NOTE: there is also an Enum called direction that I have omitted.

public class ByScoreThenName implements Comparator<Journal> {

    Direction direction;

    public ByScoreThenName(){
        this.direction = Direction.ASC;
    }

    public ByScoreThenName(Direction direction){
        this.direction = direction;
    }

    @Override
    public int compare(Journal j1, Journal j2) {

        int comparatorValue = 0;

        if (j1.getScore() == j2.getScore()) {
            comparatorValue = j2.getName().compareToIgnoreCase(j1.getName());
        } else {
            if ((j1.getScore() < j2.getScore())) {
                comparatorValue = -1;
            } else if (j1.getScore() > j2.getScore()) {
                comparatorValue = 1;
            }
        }
        return direction.coefficent() * comparatorValue;
    }
}

This code ranks numerically the journals based on their score, setting the journals rank in the journal object:

public class Numerical implements Rank<Journal> {   
    @Override
    public void doRank(List<Journal> journals) {
        for(Journal journal : journals){
           int index = journals.lastIndexOf(journal);
           if(index != 0 && journals.get(index-1).getScore() == journal.getScore()){
               journal.setRank(journals.get(index-1).getRank());
           } else {
               journal.setRank(++index);
           }
        }
    }      
}

This filters out the review journals. It implements a predicate interface:

public class OutReview implements Predicate<Journal>{
    @Override
    public boolean apply(Journal j) {
        return !j.isReview();
    }
}

And finally the journal object:

public class Journal implements Serializable{

    private static final long serialVersionUID = 8964756258469682390L;      
    private String name;
    private float score;
    private boolean review;
    private int rank;    

    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }
    public float getScore() {
        return score;
    }
    public void setScore(float score) {
        this.score = score;
    }
    public boolean isReview() {
        return review;
    }
    public void setReview(boolean review) {
        this.review = review;
    }
    public int getRank() {
        return rank;
    }
    public void setRank(int rank) {
        this.rank = rank;
    }       
    public int hashCode() {
         // Code omitted for brevity
    }              
    public boolean equals(Object obj) {
         // Code omitted for brevity
    }        
    @Override
    public String toString() { 
         // Code omitted for brevity
    }        
}

I would appreciate any kind of feedback big or small.

EDIT:

I have followed the advise given below and implemented the from() method as a static factory method as follows:

public static <T> Collation<T> from(List<T> items) {
    return new Collation<T>(new ArrayList<T>(items));
}

I have changed the names of the class and variables following the given advice:

The Direction Enum class is now imported as a static import. This cleans the code a bit.

And the array list field is now private.

\$\endgroup\$
5
\$\begingroup\$

Lambdas

I know that lambdas in Java 8 would make the code much cleaner. What other improvements are available?

I am not sure I agree with the Lambdas in J8 statement. They have their place, but clean code is readable code, and only a small subset of problems have improved readability from Lambdas, and also, Lambdas are a 'meta level' function.... Implementing 'foundation logic' using Lambdas would be a mistake because then integrating with traditional Java would be harder.

Right tool for the job, etc. In this case, I don't think Lambdas would necessarily be the right tool, even in Java8.

Specifically, your code requires cross-referencing Journals against each other (to sort them), and this would be very messy in a Stream.

Generics

You have collections of Journal throughout your code.... This is not a problem, but what I have a concern about is the inconsistency of your own Generic classes. For example, you have:

public class OutReview implements Predicate<Journal>{

and

public class Numerical implements Rank<Journal> {

Which make specific implementations of generic Interfaces.

I have no problem with this, it is nice, and clean.

Then you also have:

public class Collate<T> {

This is a generic class that has this neat ability to collate generically typed data based on matching Rank and Predicate instances....

On it's own, it is nice, clean, and general purpose.

But, you put them together, and you have a bit of a mess:

List<Journal> journals = // add journals to collection.
Collate<Journal> collateJournals = new Collate<>();
Collate<Journal> collatedJournals = collateJournals.from(journals)
      .filter(new OutReview())
      .sort(new ByScoreThenName(Direction.ASC))
      .rank(new Numerical());

For consistency, the Collate class should be hard-coded to work with Journal, or your actual OutReview and other classes should be generic too.

Naming

List<T> collection = new ArrayList<>();

Don't call a List collection. It makes the following line very confusing:

Collections.sort(collection, sortComparator);

Because Collections.sort( .... ) has only got a List<> version of the argument.

I would recommend a name like "journals" if you make the Collate class Journal specific instead of generic. Alternatively, something that describes the content, not the structure... like 'data', or 'values'.

Leaks

You have a few encapsulation leaks. The biggest is this one:

public Collate<T> from(List<T> collection) {
    this.collection = collection;
    return this;
}

you initialize your class with an empty ArrayList but then you throw it away and replace it with the from()'s argument.

Any changes outside your class to that collection (bad name) will now also be reflected in your class. For example, you may sort your collection, but then someone can add stuff out of order.....

You should be copying the collection content, not the actual reference:

public Collate<T> from(List<T> input) {
    data.clear();
    data.addAll(input);
    return this;
}    

This will also save some interesting and embarrassing bugs like when someone calls your from method with:

collate.from(Collections.unmodifiableList(somedata));
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your answer, especially the point about the leaks. I learnt a lot. \$\endgroup\$ – Alex Mar 20 '14 at 8:27
4
\$\begingroup\$
  • When using a generic name for a field (which is often fine), avoid the names of the actual type's superclasses: collection -> items since Collection lacks the ordering semantics of List.

  • Name classes as nouns instead of verbs: Collate -> Collation.

  • Collate.from should be a static factory method. There's no reason to overwrite the existing collation to save creating a new one. There's no guarantee that the given list is sorted correctly, but at least make a defensive copy.

    public static Collation<T> from(List<T> items) {
        return new Collation(new ArrayList<>(items));
    }
    

    Otherwise you risk encapsulation bugs:

    List<Book> books = ...;
    Collation<Book> booksByAuthor = Collation.from(books);
    booksByAuthor.order(Journal.BY_AUTHOR);
    books.add(new Book(... "Aaron Adams Anderson" ...);
    UI.displayLastPage(booksByAuthor);  // oops
    
  • Make use of Ordering from Guava for easy comparator creation. If not, spell out ASCENDING and DESCENDING; add some constants; and employ static imports to reduce clutter.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I implemented the static factory method. See edit to question. \$\endgroup\$ – Alex Mar 20 '14 at 8:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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