0
\$\begingroup\$

I want to be able to represent days of the week as Strings and numerical values so I can print out the days of the week and sort them properly (sun-sat instead of sorting them alphabetically). This is the first time I've ever done enumeration and I want to make sure I am doing it in the way that a professional Java dev would do it. Any feedback is appreciated.

Side Note: My teacher loves comments they are for him, I know this is too much useless commenting.

enum DayOfWeek {
    //Days of week and values associated with them.
    SUNDAY(1) {
        public DayOfWeek next() { return MONDAY; }
    }, MONDAY(2) {
        public DayOfWeek next() { return TUESDAY; }
    }, TUESDAY(3) {
        public DayOfWeek next() { return WEDNESDAY; }
    }, WEDNESDAY(4) {
        public DayOfWeek next() { return THURSDAY; }
    }, THURSDAY(5) {
        public DayOfWeek next() { return FRIDAY; }
    }, FRIDAY(6) {
        public DayOfWeek next() { return SATURDAY; }
    }, SATURDAY(7) {
        public DayOfWeek next() { return SUNDAY; }
    };

    //Instance Variables
    //Associate a number with the day of the week.
    private int dayNumber;

    //Constructors
    DayOfWeek(int dayNumber) {
        this.dayNumber = dayNumber;
    }//End of one-arg constructor.

    //Getters and Setters
    public int getDayNumber() {
        return this.dayNumber;
    }
    public void setDayNumber(int dayNumber) {
        this.dayNumber = dayNumber;
    }

    //Abstract method to implement in the enum values.
    public abstract DayOfWeek next();

    //Utility Methods
    public String toString() {
        switch(this.dayNumber) {
        case 1:
            return "Sunday";
        case 2:
            return "Monday";
        case 3:
            return "Tuesday";
        case 4:
            return "Wednesday";
        case 5:
            return "Thursday";
        case 6:
            return "Friday";
        default:
            return "Saturday";
        }
    }//End of toString method.
}//End of enum type.

public class Day {
    //Instance Variables
    DayOfWeek day;

    //Constructors
    public Day(DayOfWeek day) {
        this.day = day;
    }//End of one-arg constructor.

    public Day(int dayNumber) {
        //Instantiate the variable, then change the value to specified value.
        this.day = DayOfWeek.SUNDAY;
        this.day.setDayNumber(dayNumber);
    }//End of one-arg constructor.

    //Getters and Setters
    public DayOfWeek getDay() {
        return this.day;
    }

    //Utility Methods
    public int compareTo(Day day) {
        //Returns 1 if it comes later in the week.
        if(this.day.getDayNumber() > day.getDay().getDayNumber()) {
            return 1;
        //Returns -1 if it comes earlier in the week.
        } else if(this.day.getDayNumber() < day.getDay().getDayNumber()) {
            return -1;
        //Returns 0 if they are the same day.
        } else {
            return 0;
        }
    }//End of compareTo method.

    public String nextDay() {
        return this.day.next().toString();
    }//End of nextDay method.

    public String toString() {
        return this.day.toString();
    }//End of toString method.
}//End of class.

This is the tester class I used to test this Day class.

public class DayTester {
    public static void main(String[] args) {
        Day sunday = new Day(DayOfWeek.SUNDAY);
        Day monday = new Day(DayOfWeek.MONDAY);
        Day tuesday = new Day(DayOfWeek.TUESDAY);
        Day wednesday = new Day(DayOfWeek.WEDNESDAY);
        Day thursday = new Day(DayOfWeek.THURSDAY);
        Day friday = new Day(DayOfWeek.FRIDAY);
        Day saturday = new Day(DayOfWeek.SATURDAY);

        Day day[] = new Day[7];
        day[0] = friday;
        //day[1] = friday;
        //day[2] = friday;
        day[1] = saturday;
        day[2] = monday;
        day[3] = sunday;
        day[4] = wednesday;
        //day[5] = friday;
        day[5] = tuesday;
        day[6] = thursday;

        printArr(day);
        printArrNextDay(day);
        selectionSortArr(day);
        printArr(day);
        printArrNextDay(day);

        Day dayOfWeek = new Day(5);
        System.out.println(dayOfWeek);
        System.out.println(dayOfWeek.toString());
        System.out.println(dayOfWeek.nextDay());
    }//End of main method.

    public static void printArr(Day[] arr) {
        System.out.print("[ ");
        for(int i = 0; i < arr.length; i++) {
            if(i == (arr.length - 1)) {
                System.out.println(arr[i].toString() + " ]");
            } else {
                System.out.print(arr[i].toString() + ", ");
            }
        }
    }//End of printArr method.

    public static void printArrNextDay(Day[] arr) {
        System.out.print("[ ");
        for(int i = 0; i < arr.length; i++) {
            if(i == (arr.length - 1)) {
                System.out.println(arr[i].nextDay() + " ]");
            } else {
                System.out.print(arr[i].nextDay() + ", ");
            }
        }
    }//End of printArrNextDay method.

    public static void selectionSortArr(Day[] arr) {
        int smallest = 0;
        for(int i = 0; i < arr.length; i++) {
            smallest = i;
            for(int j = 0; j < arr.length; j++) {
                if(arr[j].compareTo(arr[smallest]) > 0) {
                    smallest = j;
                    Day temp = arr[i];
                    arr[i] = arr[smallest];
                    arr[smallest] = temp;
                }
            }
        }
    }//End of sortArr method.
}//End of class.
\$\endgroup\$
3
\$\begingroup\$

Some thoughts about your enum:

First of all, your general approach is good. The very idea of putting the relation between days and the needed data into the enum is exactly what I'd do, too.

But, alas, there are some not-so-good points:

Enums should be immutable:

You have a setDayNumber method in your enum (even public!), which could be used to mess up the whole structure of the enum. Instead, initialize the field in the constructor and make it final:

private final int dayNumber;

private DayOfWeek(int dayNumber) {
    this.dayNumber = dayNumber;
}

// delete the setter method

Harness the full power:

Why stop at a single arg constructor and use a switch over the strings? Simply add a second arg, and set a final String for the name along with the number:

// Enum constants
SUNDAY(1, "Sunday"),
...

// Vars and constructor

private final int dayNumber;
private final String dayName;

private DayOfWeek(int dayNumber, String dayName) {
    this.dayNumber = dayNumber;
    this.dayName = dayName;
}

// new toString implementation
public String toString() {
    return dayName;
}

Simplify:

Getting a next day via an overridden method seems a little overkill. We know the stucture of an enum and we know that the constants will be put into a value array in their original order and have an ordinal() which corresponds to the index.

Thus, next() would probably look like this for me:

public DayOfWeek next() {
    return DayOfWeek.values()[(ordinal() + 1) % DayOfWeek.values().length]; // or simply 7 instead of constant array length
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback :)! I did everything you said and it really helped clean up the code. I like your toString implementation because it prints out the day of the week the way I want it to print out just by printing the enum value. I also wrote a getDayName function, is there any point in doing this? It seems like it's just duplicate code, but I've been told to use getters for all immutable variables. @mtj \$\endgroup\$ – David Sep 1 '17 at 5:15
  • \$\begingroup\$ You mean getDayName() as an alternative to toString() with the same result? It depends on your interpretation of toString: if you see toString as the supplier function for the user interface, this is duplicating code. But seperating toString as a "technical" function, e.g. to see a good representation of an object in the debugger or application logs and a different function like getDayName for the user interaction has its merits, too. Especially, if you use things like localization sometimes in the future, you should definitely go for different functions. Thus, sounds good: keep it so :) \$\endgroup\$ – mtj Sep 1 '17 at 5:21
2
\$\begingroup\$

Just one little remark:

public DayOfWeek nextDay() {
    DayOfWeek[] all = values();
    return all[(ordinal() + 1) % all.length];
}

This frees you from implementing that method for every enum constant individually.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback :)! I wrote the nextDay() function this way even though I could have done what @mtj suggested and wrote return DayOfWeek.values()[(ordinal() + 1) % DayOfWeek.values().length]; This way is just easier to read and understand atm. \$\endgroup\$ – David Sep 1 '17 at 5:18
1
\$\begingroup\$
  1. public Day(int dayNumber) {
         //Instantiate the variable, then change the value to specified value.
         this.day = DayOfWeek.SUNDAY;
         this.day.setDayNumber(dayNumber);
    }//End of one-arg constructor.
    

    Why are you setting the day just to change it directly below? It doesn't make sense at all. (the problem isn't that you change it directly below, the problem is that you are setting day to SUNDAY without any reason)

  2. You shouldn't switch on enums (DayOfWeek.toString). I'd personally fix this by just lowercasing the whole name() except the 1st letter though you can also just make this a constructor param.
  3. You could replace the dayNumber with just ordinal() but that's up to you to decide (you could just leave the variable, leave it but change the constructor to a no-arg one and initialize the as dayOfWeek = ordinal() or just use ordinal() throughout the enum)
  4. Wtf is this?

    Day day[] = new Day[7];
    day[0] = friday;
    //day[1] = friday;
    //day[2] = friday;
    day[1] = saturday;
    day[2] = monday;
    day[3] = sunday;
    day[4] = wednesday;
    //day[5] = friday;
    day[5] = tuesday;
    day[6] = thursday;
    

    I assume this is just for testing? (if you'd apply the 5th suggestion you could basically just do DayOfWeek.values() to achieve the same effect)

  5. I kind of dislike your DayOfWeek and Day separation. You could easily do this using only the enum
  6. In the Day class you have a compareTo method but you aren't implementing Comparable<Day>. If you have that method then why aren't you implementing the interface?
  7. public String nextDay() - why aren't you returning a Day? The name nextDay should either be changed (not preferred) or you should return a Day (much better).
  8. Why can you change your enum's day number? It's at least a red flag for me. If you wanted to support both Monday as first day and Sunday then you definitely shouldn't do that like this.
  9. for(int i = 0; i < arr.length; i++) {
        if(i == (arr.length - 1)) {
            System.out.println(arr[i].toString() + " ]");
        } else {
            System.out.print(arr[i].toString() + ", ");
        }
    }
    

    a) you don't need the toString

    b) it's not exactly bad but I believe you could do this in a more readable way - loop over arr.length - 1, remove the if and then after the for loop place System.out.println(arr[arr.length - 1] + "]")

\$\endgroup\$
  • \$\begingroup\$ 1. I had to initialize to something before I set it to the int parameter or else I got a NullPointerException and I decided to just initialize it to the first value of the enum. I removed it completely. I wanted to have the option of creating a day object with just an int. 2. I simplified the toString public String toString() { return this.name().substring(0,1).toUpperCase() + this.name().substring(1).toLowerCase(); } \$\endgroup\$ – David Aug 31 '17 at 0:21
  • \$\begingroup\$ @David I read those 2 lines a few times and I understand it now. What I do not understand though is why do you need that? \$\endgroup\$ – Mibac Aug 31 '17 at 0:23
  • \$\begingroup\$ I'm just going to answer your answer with another answer. I can't fit it all in a comment. \$\endgroup\$ – David Aug 31 '17 at 0:25
  • \$\begingroup\$ But why do you need to change the DayOfWeek's day number? @David \$\endgroup\$ – Mibac Aug 31 '17 at 0:29
  • \$\begingroup\$ Ok, I responded to you with an answer so you can see my updated code. Thank you for the help :). Let me know if there is anything else I should be doing, but aren't. @Mibac \$\endgroup\$ – David Aug 31 '17 at 0:43
0
\$\begingroup\$
  1. I had to initialize to something before I set it to the int parameter or else I got a NullPointerException and I decided to just initialize it to the first value of the enum. I removed it completely. I wanted to have the option of creating a day object with just an int. I don't know why I just thought it'd be cool to have more options.

  2. I simplified the toString code:

    public String toString() { return this.name().substring(0,1).toUpperCase() + this.name().substring(1).toLowerCase(); }//End of toString method.

  3. I'm not sure how to write a no-arg constructor for an enum. Could you provide an example?

  4. I needed to create an array that was out of order so I could make sure to sort it. Doing DayOfWeek[] arr = DayOfWeek.values(); makes it sorted already.

  5. I'm still a little confused about how enums work, hard to find a thorough explanation online. I made it so it is just an enums class called DayOfWeek (code below).

  6. Didn't know about the Comparable interface. Can I implement interfaces with an enum class?

  7. Removed that. Wasn't needed when I just use an enum class (code below).

  8. I made the number value final. I have just been told to make my code as flexible as possible, but yeah I agree with you that's a bad idea (code below). I don't actually need to change the day number and prefer to just have it as a set value.

  9. Fixed all of that (code below).

I posted the code below for the updated enum class. I'm not using a Day class anymore to use the enum. Is the only way to declare an enum like this?

DayOfWeek day = DayOfWeek.SUNDAY;

Or is there a way to use the constructor like a regular object?

DayOfWeek day2 = new DayOfWeek(1);

public enum DayOfWeek {
    //Days of week and values associated with them.
    SUNDAY(1) {
        public DayOfWeek nextDay() { return MONDAY; }
    }, MONDAY(2) {
        public DayOfWeek nextDay() { return TUESDAY; }
    }, TUESDAY(3) {
        public DayOfWeek nextDay() { return WEDNESDAY; }
    }, WEDNESDAY(4) {
        public DayOfWeek nextDay() { return THURSDAY; }
    }, THURSDAY(5) {
        public DayOfWeek nextDay() { return FRIDAY; }
    }, FRIDAY(6) {
        public DayOfWeek nextDay() { return SATURDAY; }
    }, SATURDAY(7) {
        public DayOfWeek nextDay() { return SUNDAY; }
    };

    //Instance Variables
    //Associate a number with the day of the week.
    private final int dayNum;

    //Constructors
    DayOfWeek(int dayNum) {
        this.dayNum = dayNum;
    }//End of one-arg constructor.

    //Getters and Setters
    public int getDayNum() {
        return this.dayNum;
    }

    //Abstract method to implement in the enum values.
    public abstract DayOfWeek nextDay();

    //Utility Methods
    public int compareToDay(DayOfWeek day) {
        //Returns 1 if it comes later in the week.
        if(this.ordinal() > day.ordinal()) {
            return 1;
        //Returns -1 if it comes earlier in the week.
        } else if(this.ordinal() < day.ordinal()) {
            return -1;
        //Returns 0 if they are the same day.
        } else {
            return 0;
        }
    }//End of compareTo method.

    public String toString() {
        return this.name().substring(0,1).toUpperCase() + this.name().substring(1).toLowerCase();
    }//End of toString method.
}//End of enum type.

This is my updated test class. I realized I don't need to write my own compareTo method because the one that comes with an enum class works the same.

public class DayOfWeekTester {
    public static void main(String[] args) {
        DayOfWeek day = DayOfWeek.SUNDAY;
        System.out.println(day);
        System.out.println(day.ordinal());
        System.out.println(day.getDayNum());
        System.out.println();

        DayOfWeek week[] = DayOfWeek.values();
        printArr(week);
        System.out.println();

        DayOfWeek sunday = DayOfWeek.SUNDAY;
        DayOfWeek monday = DayOfWeek.MONDAY;
        DayOfWeek tuesday = DayOfWeek.TUESDAY;
        DayOfWeek wednesday = DayOfWeek.WEDNESDAY;
        DayOfWeek thursday = DayOfWeek.THURSDAY;
        DayOfWeek friday = DayOfWeek.FRIDAY;
        DayOfWeek saturday = DayOfWeek.SATURDAY;
        DayOfWeek unsortedWeek[] = new DayOfWeek[7];
        unsortedWeek[0] = friday;
        unsortedWeek[1] = monday;
        unsortedWeek[2] = sunday;
        unsortedWeek[3] = saturday;
        unsortedWeek[4] = wednesday;
        unsortedWeek[5] = tuesday;
        unsortedWeek[6] = thursday;
        printArr(unsortedWeek);
        //selectionSort(unsortedWeek);
        selectionSortRegularCompareTo(unsortedWeek);
        printArr(unsortedWeek);
    }//End of main method.

    public static void selectionSortRegularCompareTo(DayOfWeek[] arr) {
        int smallest = 0;
        for(int i = 0; i < arr.length; i++) {
            smallest = i;
            for(int j = 0; j < arr.length; j++) {
                if(arr[j].compareTo(arr[smallest]) > 0) {
                    smallest = j;
                    DayOfWeek temp = arr[i];
                    arr[i] = arr[smallest];
                    arr[smallest] = temp;
                }
            }
        }
    }//End of selectionSort method.

    public static void selectionSort(DayOfWeek[] arr) {
        int smallest = 0;
        for(int i = 0; i < arr.length; i++) {
            smallest = i;
            for(int j = 0; j < arr.length; j++) {
                if(arr[j].compareToDay(arr[smallest]) > 0) {
                    smallest = j;
                    DayOfWeek temp = arr[i];
                    arr[i] = arr[smallest];
                    arr[smallest] = temp;
                }
            }
        }
    }//End of selectionSort method.

    public static void printArr(DayOfWeek[] arr) {
        System.out.print("[ ");
        for(int i = 0; i < arr.length - 1; i++) {
            System.out.print(arr[i] + ", ");
        }
        System.out.println(arr[arr.length - 1] + " ]");
    }//End of printArr method.
}//End of class.
\$\endgroup\$
  • \$\begingroup\$ 3. DayOfWeek() { /* code */ this.dayNum = ordinal() + 1; } and then in the enum's element's: SOMETHING, SOMETHING_ELSE, ETC 5. The enum looks okay 6. Yes. You implement it as if it was a class (enum XYZ implements ABC) \$\endgroup\$ – Mibac Aug 31 '17 at 0:45
  • \$\begingroup\$ Each enum element can have only one instance (they are basically singletons), so no, you can't do new on enums \$\endgroup\$ – Mibac Aug 31 '17 at 0:48
  • \$\begingroup\$ You do know that your selectionSort and selectionSortRegularCompareTo are exactly the same, right? \$\endgroup\$ – Mibac Aug 31 '17 at 0:49
  • \$\begingroup\$ Yes, one uses the compareTo that is just part of enums and one uses the compareToDay method I wrote. They both achieve exactly the same thing so I got rid of my compareToDay method. I was just testing to make sure it did what I wanted. @Mibac \$\endgroup\$ – David Aug 31 '17 at 0:54
  • \$\begingroup\$ Better go for a constructor param for toString than your point #2: the implementation with lowercasing the rest of the string ties the visual name for the end-user to the technical name of the enum constant. And if you want to change the display name, let's say to "first day of week" you have to redo the inner workings of the code. Avoid that. \$\endgroup\$ – mtj Aug 31 '17 at 12:07

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.