current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I have a list of objects with a string (name) and a local date (date of deletion).

I wish to implement uniqueness on these objects based on:

  • If two objects have the same name, then I wish the one with no date of deletion to have priority.
  • If two objects have the same name, and a date of deletion, then I wish the one with the most recent date of deletion to have priority.

Based on the above criteria, I have implemented, what I think is the right solution.

Do you have any pointers as to the correctness of my approach? Do you have any suggestions for improvements?

The class:

public class Unique implements Comparable<Unique>
{
    private String name;
    private LocalDate dateOfDeletion;

    public Unique(String name, LocalDate dateOfDeletion) {
        this.name = name;
        this.dateOfDeletion = dateOfDeletion;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public LocalDate getDateOfDeletion() {
        return dateOfDeletion;
    }

    public void setDateOfDeletion(LocalDate dateOfDeletion) {
        this.dateOfDeletion = dateOfDeletion;
    }

    @Override
    public String toString() {
        return "Unique [name=" +name + ", dateOfDeletion="+dateOfDeletion+"],";
    }

    /**
     * order based on name ascending, and date descending
     */
    @Override
    public int compareTo(Unique rhs) {  
        Comparator<Unique> reverseDateComparator = (a, b) -> {
            LocalDate thisDodel = a.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : a.getDateOfDeletion();
            LocalDate thatDodel = b.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : b.getDateOfDeletion();
            return thatDodel.compareTo(thisDodel);
        };
        return Comparator.comparing(Unique::getName).thenComparing(reverseDateComparator).compare(this, rhs);
    }
}

The Test:

public static void main(String[] args) {
    List<Unique> uniqueList = Arrays.asList(
                new Unique("a", null) 
                ,new Unique("b", null) 
                ,new Unique("b", LocalDate.of(2016, Month.SEPTEMBER, 20))
                ,new Unique("c", LocalDate.of(2016, Month.SEPTEMBER, 19))
                ,new Unique("c", LocalDate.of(2016, Month.SEPTEMBER, 20))
                );

    System.out.println("list ordered, with duplicates: ");
    uniqueList.stream().sorted().forEach(System.out::println);

    System.out.println("=================");
    System.out.println("list ordered, duplicates removed: ");
    Comparator<Unique> uniqueComp = (a, b) -> a.getName().compareTo( b.getName() );
    //SOLUTION HERE: since list is already ordered by date desc, it will ignore the records with same name already added to the list  
    TreeSet<Unique> sortedFilterd = uniqueList.stream().sorted().collect(Collectors.toCollection(() -> new TreeSet<Unique>(uniqueComp)));
    sortedFilterd.forEach(System.out::println);
}

What I expect to see is the following:

list ordered, with duplicates: 
Unique [name=a, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=2016-09-20],
Unique [name=c, dateOfDeletion=2016-09-20],
Unique [name=c, dateOfDeletion=2016-09-19],
=================
list ordered, duplicates removed: 
Unique [name=a, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=null],
Unique [name=c, dateOfDeletion=2016-09-20],

Here is a link to the working sample: Link to working sample to the above code

share|improve this question
up vote 2 down vote accepted

Simplification

The code can be simplified using built-in methods from the Comparator API.

public int compareTo(Unique rhs) {  
    Comparator<Unique> reverseDateComparator = (a, b) -> {
        LocalDate thisDodel = a.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : a.getDateOfDeletion();
        LocalDate thatDodel = b.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : b.getDateOfDeletion();
        return thatDodel.compareTo(thisDodel);
    };
    return Comparator.comparing(Unique::getName).thenComparing(reverseDateComparator).compare(this, rhs);
}

The reverseDateComparator is primarily used to handle null values. However, there are the built-ins comparators nullsFirst and nullsLast that exist just for this: they wrap an existing comparator and turn it null safe, by considering null values either greater or lower than non-null values.

As such, you can rewrite this method with:

@Override
public int compareTo(Unique rhs) {  
    return Comparator.comparing(Unique::getName)
                .thenComparing(Unique::getDateOfDeletion, Comparator.nullsFirst(Comparator.reverseOrder()))
                .compare(this, rhs);
}

This directly passes the method-reference Unique::getDateOfDeletion to thenComparing and compares the dates by placing null values before other values and non-null in their reverse natural order (so that the most recent date is sorted before).

Using static imports makes the code a bit cleaner also. With the following imports,

import static java.util.Comparator.comparing;
import static java.util.Comparator.nullsFirst;
import static java.util.Comparator.reverseOrder;

the methods becomes very readable with regard to the initial goal (compare by name, then compare by dates reversed with first no deletion date):

@Override
public int compareTo(Unique rhs) {  
    return comparing(Unique::getName)
                .thenComparing(Unique::getDateOfDeletion, nullsFirst(reverseOrder()))
                .compare(this, rhs);
}

In the same way, the following:

Comparator<Unique> uniqueComp = (a, b) -> a.getName().compareTo( b.getName() );

can be written:

Comparator<Unique> uniqueComp = Comparator.comparing(Unique::getName);

equals needs to be overriden

Since your Unique class implements Comparable, it is strongly recommended to let it override the equals method, and have a consistent implementation.

In this case, the equals should simply verify the equality of both the names of the date of deletion. To handle null values, you can make use of Objects.equals: this method returns true when the two given objects are either both null or both not-null and equal with regard to equals.

Other comments

  • Do you really need setters in your Unique class? Consider making that class immutable.

  • You have a trailing comma in

    return "Unique [name=" +name + ", dateOfDeletion="+dateOfDeletion+"],";

  • Does it make sense to allow a null name? If not, you can validate that it is always present in the constructor with requireNonNull. Also, since the date of deletion might be null, a constructor overload without it may be simpler for the client.

share|improve this answer
    
If I could give you more than one upvote, I would. The recommendations are simple, yet nothing short of genius. =0) I couldn't find an example of how to properly implement Comparator.nullsFirst(). Yours is just what I was looking for. What I wanted to ask is does stream().sorted().collect(Collectors.toCollection(() -> new TreeSet<Unique>(uniqueComp))); is enough to remove duplicates? Is there a better way to implement this simple solution? Thank you for the recommendations. – Lucas T Sep 21 '16 at 14:32
    
@LucasT Yes it's enough, it will retain uniques with different names. It will always keep the first one in the input list (even if ran in a parallel stream) – Tunaki Sep 21 '16 at 14:47

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.