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

I have implemented a class which implements a list of my custom DateObj. I have sorted the list in a peculiar manner based on the current month. I achieved my desired results, but I'm required to call Collections.sort() four times to achieve it. I feel this could be improved. Any thoughts?

//Custom Date Object
public class DateObj {

private int year;
private int month;
private int date;

DateObj(int year, int month ,int date){
    this.year = year;
    this.month = month;
    this.date = date;
}

public Integer getYear() {return year;}
public Integer getMonth() {return month;}
public Integer getDate() {return date;}
}

The custom sort class:

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

public class CustomListSort {
 static List<DateObj> list = new ArrayList<>();

public static void main(String a[]){

createRandomDateObjs();

Integer CURRENTMONTH = 6;

//Sort Months Descending
Collections.sort(list,(arg0,arg1)-> arg1.getMonth().compareTo(arg0.getMonth()));

//Sort by keeping the object with nearest largest next month first
Collections.sort(list,(arg0,arg1)->CURRENTMONTH.compareTo(arg0.getMonth()));

//Sort the months which were pushed back in an ascending order.
Collections.sort(list,(arg0,arg1)->(arg0.getMonth()<=CURRENTMONTH && arg1.getMonth()<=CURRENTMONTH)? arg0.getMonth().compareTo(arg1.getMonth()):0);

//If months same sort them in ascending manner based on their dates
Collections.sort(list, (arg0,arg1)->(arg0.getMonth() == arg1.getMonth())  ? arg0.getDate().compareTo(arg1.getDate()) : 0);

System.out.println("\nDESIRED OUTPUT");
System.out.println("\nMM - DD - YYYY \n");

for(int i = 0 ; i<list.size();i++)
    System.out.println(list.get(i).getMonth() +" - "+list.get(i).getDate()+" - "+list.get(i).getYear());        
}

static void createRandomDateObjs(){
    for(int i = 0 ; i<20 ; i++){
        int y = 1980;
            y+=(int)(Math.random()*55);
        int m = (int) (Math.random()*11);
        int d = (int) (Math.random()*30);
        list.add(new DateObj(++y,++m,++d)); //Pre-increment to change 0's to 1's
}
}

Initially I had implemented it using Comparators but the Lambda made the code much cleaner.

Desired output:

MM - DD - YYYY 

7 - 20 - 2023
7 - 18 - 2027
8 - 30 - 2010
8 - 12 - 2020
9 - 21 - 2006
10 - 23 - 2008
11 - 19 - 2000
11 - 16 - 2033
11 - 13 - 1989
12 - 13 - 2019
1 - 10 - 1985
1 - 15 - 2016
4 - 10 - 2021
4 - 13 - 2022
4 - 14 - 2004
5 - 1 - 2025
5 - 4 - 2014
5 - 20 - 2023
5 - 21 - 2022
6 - 29 - 1990
share|improve this question
5  
You've been told on your stack overflow question that you can use Comparator.thenComparing on Java 8 to achieve this... – Vogel612 Jan 24 at 12:24
up vote 5 down vote accepted

A Comparator is the correct approach, you just need to mod the month by the current month before comparing them. The easiest way to do this is to introduce a static variable currentMonth to hold the current month for all instances of DateObj.

Add DateObj.setCurrentMonth(CURRENTMONTH); to your Main, and use the code below:

//Custom Date Object
class DateObj implements Comparable<DateObj>{

    private int year;
    private int month;
    private int date;
    private static int currentMonth;

    DateObj(int year, int month ,int date){
      this.year = year;
      this.month = month;
      this.date = date;
    }

    public Integer getYear() {return year;}
    public Integer getMonth() {return month;}
    public Integer getDate() {return date;}
    public Integer getCurrentMonth() { return DateObj.currentMonth;}
    public static void setCurrentMonth(int currentMonth){
        DateObj.currentMonth = currentMonth;
    }

    @Override
    public int compareTo(DateObj o) {
        int months = (month+(12-currentMonth))%12 - (o.month+(12-currentMonth))%12;
        if(months == 0)
            return this.date-o.date;
        else{
            return months;
        }
    }
}

EDIT: An alternative approach is to simply define the Comparator in your main body. This avoids the use a static value to hold currentMonth:

    Collections.sort(list, (o1,o2)->{
            int months = (o1.getMonth()+(12-CURRENTMONTH))%12 - (o2.getMonth()+(12-CURRENTMONTH))%12;
            if(months == 0)
                return o1.getDate()-o2.getDate();
            else{
                return months;
            }
    });

Thanks to Mr.Wiggles for the recommendation.

share|improve this answer

Your current code has several flaws:

  • Do not return an Integer object when an int suffice. It might create an unnecessary object. I would rewrite your getter like this

    public int getYear() {
        return year;
    }
    
  • You should properly indent and format your code, as it makes it a lot easier to read. For example, the getter can be written more clearly like the snippet above.
  • You are indeed comparing 4 times the list when you can do it in one pass.
  • Your usage of static is awkward. You should get rid of it and make the method createRandomDateObjs() return the created list of DateObj.
  • Don't name your local variable CURRENTMONTH. If it is a constant, make it private static final int instead.

The heart of the comparator is about comparing the month of the dates. Instead of using 3 separate comparator, consider the following one. Given 2 DateObj o1 and o2:

  • If the months of o1 and o2 are both strictly greater than 6, the one being closer to 6 should be sorted before the other one.
  • If the months of o1 and o2 are both lower or equal to 6, the one being closer to 6 should be sorted after the other one.

(Those two conditions takes care of the fact that the month 6 should be sorted at the end of the list)

  • If they have different signs, we need an extra logic:
    • If the month of o1 is strictly after 6 (so the month of o2 is before 6), it means that o1 should be sorted before o2 so we must return a negative value (like -1).
    • On the contrary, if the month of o1 is before or equal to 6 (so the month of o2 is after 6), it means that o1 should be sorted after o2 so we must return a positive value (like 1).

This would an implementation of this comparator using a lambda expression:

    Comparator<DateObj> comparator = (o1, o2) -> {
        int diff1 = o1.getMonth() - CURRENTMONTH;
        int diff2 = o2.getMonth() - CURRENTMONTH;
        if (diff1 > 0 && diff2 > 0 || diff1 <= 0 && diff2 <= 0) {
            return diff1 - diff2;
        } else if (diff1 > 0) {
            return -1;
        } else if (diff1 <= 0) {
            return 1;
        }
        return 0;
    };

The advantage with this reasoning over using modular arithmetic is that it will work for whatever value (even negative value). The answer by Austin D assumes that all value are positive and between 0 and 11 (it could be made mode flexible but then you would first have to determine the maximum value). It only works for this particular case (because a month always verifies that assumption).

Then, in case the month are equal, your code is sorting based on the date field. With Java 8, you can compose multiple comparator with Comparator.thenComparing. In this case, we will then compare with an int so we can use Comparator.thenComparingInt(keyExtractor), where the key extractor returns the date.

The final comparator would be:

    Comparator<DateObj> comparator = (o1, o2) -> {
        int diff1 = o1.getMonth() - CURRENTMONTH;
        int diff2 = o2.getMonth() - CURRENTMONTH;
        if (diff1 > 0 && diff2 > 0 || diff1 <= 0 && diff2 <= 0) {
            return diff1 - diff2;
        } else if (diff1 > 0) {
            return -1;
        } else if (diff1 <= 0) {
            return 1;
        }
        return 0;
    };
comparator = comparator.thenComparingInt(DateObj::getDate);

list.sort(comparator);
share|improve this answer

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.