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.

I've created a Java class, Date, in which I basically create date strings and set dates. Is there anything I can do besides adding comments to improve/shorten my code?

I've tested all of my methods to see if they work correctly, and they all work as planned. I haven't done tests that force the IllegalArgumentsException to be thrown yet though.

I've tried changing the instantiated boolean variable shortDisplay have a default value of true, which has made my program work correctly as far as I know.

I'm assuming when you declare a boolean instance variable, you need to also declare what its default value should be, otherwise issues arise. If anyone could comment on why that is, that would be great.

Date.java

public class Date {
    private int year;
    private int month;
    private int day;
    private boolean shortDisplay = true;

    public Date() {
        setDate(2000, 01, 01);
    }

    public Date(int yy, int mm, int dd) {
        setDate(yy, mm, dd);
    }

    public Date(int yy, int mm) {
        setDate(yy, mm);
    }

    public Date(int yy) {
        setDate(yy);
    }

    public Date(Date otherDate) {
        setDate(otherDate);
    }

    public Date(String dateStr) {
        setDate(dateStr);
    }

    public void setDate(int yy, int mm, int dd) {
        setYear(yy);
        setMonth(mm);
        setDay(dd);
    }

    public void setDate(int yy, int mm) {
        setYear(yy);
        setMonth(mm);
        setDay(1);
    }

    public void setDate(int yy) {
        setYear(yy);
        setMonth(1);
        setDay(1);
    }

    public void setDate(Date otherDate) {
        int yy = getYear();
        int mm = getMonth();
        int dd = getDay();
        setYear(yy);
        setMonth(mm);
        setDay(dd);
    }

    public void setDate(String dateStr) {
        int length = dateStr.length();
        int indexCount = 0;
        if (length >= 0) { // basically checking to see if the date even exists
            if (dateStr.indexOf('/') >= 0) {
                int index = dateStr.indexOf('/');
                indexCount++;
                String dd = dateStr.substring(index+1, dateStr.length());
                if (dd.substring(1, dd.length()).lastIndexOf('/') >= 0 ) {
                    int index2 =  dd.lastIndexOf('/');
                    indexCount++;
                    String yy = dd.substring(index2+1, dd.length());
                    if (yy.lastIndexOf('/') >= 0) {
                        indexCount++;
                    }
                    String mm = dateStr.substring(0, index);
                    dd = dd.substring(0, index2);
                    yy = yy.substring(0, yy.length());
                    System.out.println(mm + "\n" + dd + "\n" + yy );
                    int y = Integer.parseInt(yy);
                    int d = Integer.parseInt(dd);
                    int m = Integer.parseInt(mm);
                    setYear(y);
                    setMonth(m);
                    setDay(d);
                }
            }
        }

    }

    public void setYear(int yy) {
        if (yy >= 1900) {
            year = yy;
        } else {
            throw new IllegalArgumentException("lol");
        }
    }

    public void setMonth(int mm) {
        if (mm >= 1 && mm <= 12) {
            month = mm;
        } else {
            throw new IllegalArgumentException("not cool");
        }
    }

    public void setDay(int dd) {
        if (dd >= 1 && dd <= 31) {
            day = dd;
        } else {
            throw new IllegalArgumentException("stupid");
        }
    }

    public int getYear() {
        return year;
    }

    public int getMonth() {
        return month;
    }

    public int getDay() {
        return day;
    }

    public void setShortDisplay() {
        shortDisplay = true;
    }

    public void setLongDisplay() {
        shortDisplay = false;
    }

    public boolean isShortDisplay() {
        if (shortDisplay) {
            return true;
        } else {
            return false;
        }
    }

    public void incrementDay() {
        day++;
        if (day > 31) {
            day = 1;
            incrementMonth();
        }
    }

    public void incrementMonth() {
        month++;
        if (month > 12) {
            month = 1;
            incrementYear();
        }
    }

    public void incrementYear() {
        year++;
    }

    public boolean equals(Object other) {
        Date that = (Date) other;
        if (this.day == that.day && this.month == that.month && this.year == that.year) {
            return true;
        } else {
            return false;
        }
    }

    public boolean before(Date otherDate) {
        Date that = otherDate; 
        if (this.year < that.year) return true;
        if (this.month < that.month) return true;
        if (this.day < that.day) return true;
        else return false;
    }

    public String toString() {
        boolean check = isShortDisplay();
        if (check) {
            if (month < 10 && day < 10) {
                return "0" + month + "/" + "0" + day + "/" + year;
            }
            if (month < 10 && day > 10) {
                return "0" + month + "/" + day + "/" + year;
            }
            if (month > 10 || day < 10) {
                return month + "/" + "0" + day + "/" + year;
            } else {
                return  month + "/" + day + "/" + year;
            }
        } else {
            return monthString(month) + " " + day + ", " + year;
        }
    }

    public static String monthString(int month) {
        if (month >= 1 && month <= 12) {
            String[] stringMonth = {"", "January", "February", "March", "April", "June", "July",
                "August", "September", "October", "November", "December"};
        return stringMonth[month];
        } else {
            throw new IllegalArgumentException();
        }
    }
}

DateTest.java

public class DateTest {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        Date d = new Date(2013, 4, 9);
        System.out.println(d);
        d.setLongDisplay();
        System.out.println(d);

        Date d1 = new Date();
        if (!d1.toString().equals("01/01/2000")) {
            System.out.println("Error 1");
        }

        d1.setDate("4/8/2013");
        if (!d1.toString().equals("04/08/2013")) {
            System.out.println("Error 2");
        }

        d1.setDay(28);
        for (int i = 0; i<5; i++) {
            d1.incrementDay();
        }

        System.out.println(d1);

        if (!d1.toString().equals("05/02/2013")) {
            System.out.println("Error 3");
        }
    }
}
share|improve this question
2  
Naming any class such that it conflicts with a class in java.util is probably a bad idea. java.sql.Date is bad enough. –  200_success Apr 16 at 8:17
    
Post rolled back. Please don't update the code from answers; that will invalidate them. If you'd like further review, you may ask a new follow-up question. –  Jamal Apr 21 at 21:28
    
@Jamal Okay, I'll try to keep that in mind from now on. –  cbenn95 Apr 21 at 21:29

3 Answers 3

up vote 5 down vote accepted
  • Your class does not handle months with less than 31 days, in-real-life-speaking your unit test for adding five days to 28th April is already wrong
  • stringMonth can be left as a private static final array, so that you are not re-creating it every time monthString() is called
  • equals() implementation does not check for null/class type, and by right you should have a corresponding hashCode() implementation too
  • before() can probably be better implemented as compareTo() if you can consider letting your class implement Comparable
  • There are better ways of formatting the String representation in toString() and parsing a String date in setDate(String dateStr)
  • Some Javadocs will be helpful too, especially to indicate the format that you are using in toString() ("MM/DD/YY")
  • Consider a proper testing framework such as JUnit or TestNG?
share|improve this answer

Instead of putting print statements in the main method, it would be better to write proper unit tests. That way you don't have to read the output and understand if it's correct or not, the assertions will take care of that for you, so you can focus on just what fails after you changed something. For example using JUnit4:

public class DateTest {
    @Test
    public void testShortAndLongDisplay() {
        Date d = new Date(2013, 4, 9);
        Assert.assertEquals("04/09/2013", d.toString());
        d.setLongDisplay();
        Assert.assertEquals("April 9, 2013", d.toString());
    }

    @Test
    public void testSetDate() {
        Date d1 = new Date();
        Assert.assertEquals("01/01/2000", d1.toString());

        d1.setDate("4/8/2013");
        Assert.assertEquals("04/08/2013", d1.toString());
    }

    @Test
    public void testIncrementDay() {
        Date d1 = new Date(2013, 4, 28);
        Assert.assertEquals("04/28/2013", d1.toString());
        for (int i = 0; i < 5; i++) {
            d1.incrementDay();
        }
        Assert.assertEquals("05/02/2013", d1.toString());
    }
}

There is a bug in the before method, as it fails this unit test (the second assert):

@Test
public void testBefore() {
    Date d1 = new Date(2013, 4, 28);
    Date d2 = new Date(2013, 5, 27);
    Assert.assertTrue(d1.before(d2));
    Assert.assertFalse(d2.before(d1));
}

Some problems in the setDate method:

  • indexCount is incremented but never used
  • You should not print things in code with System.out.println. You might want to use a Logger instead.

If you don't plan to create sub-classes of this one, then it's better to access shortDisplay directly within the class instead of the isShortDisplay accessor. But if you do plan sub-classes then never mind.


This can be simplified:

public boolean isShortDisplay() {
    if (shortDisplay) {
        return true;
    } else {
        return false;
    }
}

Since shortDisplay is a boolean variable, you can simply return it:

public boolean isShortDisplay() {
    return shortDisplay;
}

As others pointed out, the equals method doesn't check for null and class type. Also, the if-else there can be simplified:

@Override
public boolean equals(Object obj) {
    if (obj instanceof Date) {
        Date that = (Date) obj;
        return this.day == that.day
                && this.month == that.month
                && this.year == that.year;
    }
    return false;
}

It's also nice to explicitly add the @Override annotation when you're overriding a superclass method, just in case somebody reads the code (or a diff) outside a nice IDE.

And when you implement an equals method, you should also implement a hashCode method to ensure consistency, how about:

@Override
public int hashCode() {
    return String.format("%02d/%02d/%04d", day, month, year).hashCode();
}

setDate(2000, 01, 01);

IntelliJ gives me a warning about this, because 01 is an octal integer... Very cosmetic, but maybe you can just drop those zeros anyway....

share|improve this answer
    
"because 01 is an octal integer" Good catch, this means 08 wouldn't work. ;) –  h.j.k. Apr 16 at 7:11
    
I had the idea of returning something like what your hashCode() does, but I didn't know how to properly make that work. I haven't learned about using something like String.format, so I tried using printf. Also the print statements were there because I was having issues with grabbing the right numbers from a given string date, and so I used them to see what was being created. Thanks for all the tips. –  cbenn95 Apr 16 at 22:00
    
@cbenn95 you're welcome, I hope this helped –  janos Apr 16 at 22:29
1  
@cbenn95 If you consult the Javadoc for equals() (docs.oracle.com/javase/7/docs/api/java/lang/…), one of this method's property should be that it is symmetric, x.equals(y) should return true if and only if y.equals(x). Since we can't possibly make a String object be equal to your Date object, you should not check for equality against String objects. –  h.j.k. Apr 22 at 1:29
1  
Furthermore, even if a String test = "20140422" does equal to a Date temp = new Date(2014,4,22) in non-programming terms, the way the hash code is calculated will be different between the two. It is not a convention to compare equality across different classes. It may make some sense between parent and child classes or classes meant to be used internally/strictly according to some explicit assumptions, but that's about all I can think of. –  h.j.k. Apr 22 at 1:32

I don't know why you want to write your own Date class. In Java you can use java.util.Date, or java.sql.Date with java.text.SimpleDateFormat. If you need your own Date class, you can write for instance a wrapper class or write an Util class.

Java Date API is ugly. In java 8 there is a new more usable date API. In earlier versions I recommend Joda-Time 3rd party library.

Review of Your code:

  1. Unit tests: There are tools such as JUnit or TestNG to test your code. This way you can write more reusable, simpler, easy-to-use, automated tests. The real unit tests help your work, for instance you can describe the the expected behaviour in a more proper way.

  2. Your Date class:

    • Constructors There are a lot of constructors with different number of parameters. You can "embed" them with the this(...) method. For example:

      public Date() {
      //  setDate(2000, 01, 01);
        this(2000, 1, 1);
      }
      
    • Setters

      • The setDate(...) methods can invoke each similar as I wrote in the constructor section.
      • The setDate(Date otherDate) method is not working now. It's setting the owned values now. Instead of you can use:

        public void setDate(final Date otherDate) {
            year = otherDate.year;
            month = otherDate.month;
            day = otherDate.day;
        }
        
      • The setDay(...), setYear(...), setMonth(...) methods are invoked in the class for example in the constructors or in other setters. Robert Cecil Martin: Clean Code says: One Level of Abstraction per Function in Chapter 3: functions. So you can separate the validating from the setters for example this way:

        private static final int MINIMUM_YEAR = 1900;
        
        public void setYear(final int year) {
            this.year = checkYear(year);
        }
        
        public int checkYear(final int year) {
            if (year >= MINIMUM_YEAR) {
                return year;
            }
        
            throw new IllegalArgumentException("The year must be greater than " + MINIMUM_YEAR);
        }
        

        Here 1900 was a "magic number", you should name it for increase readability. You should name your variables what they exactly. yy is ok in a date format, but as a variable it's exactly a year. Don't mind if it's hiding a field, in Java you can access your fields through this.. For example this.year.

      • The setDate(String dateStr) method is so unreadable and splitting string with indexOf is so hard. You can use String.split(...) for example to get the values.
        • The dateAsString name is more readable than dateStr.
        • The indexCount variable is not used so you can remove it.
        • As Joshua Bloch's Effective Java says: Whenever possible use final variables. This minimize the scope of local variables.
      • Your setDate(...) methods of your Date class has tricky name, because in proper way one setter sets one field. So the year, month, day field trio represents the date here. It's not a main problem, but decreasing the code's readability.
    • monthString(int month) You should use a private static array or an enum class, to don't redefine the mounts on every invocation.
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.