3
\$\begingroup\$

Here's the start of some code for a clock that can display time in 24 and 12 hours.

What could be added to it? How could I can apply notions such as OOP, model and Enums here?

public Clock {
    private int time = 0;

    public int get24hrTime(){
        return time;
    }

    public int set24hrTime( int newTime ){
        this.time = newTime;
    }

    public int get12hrTime(){
        if( time - 12 < 0 ){
            return time;
        } else {
            return time - 12;
        }
    }

    public String get12hrPostfix(){
        if( hours - 12 < 0 ){
            return "AM";
        } else {
            return "PM";
        }
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ "Do not fix something that works", what is the issue with this code? \$\endgroup\$ Aug 5, 2013 at 10:38
  • 1
    \$\begingroup\$ There is no functional issue with this code. It will get the green flag in TDD. But I am bothered by the design aspect. I just want to see how other programmers could design this more elegantly with comments on their reasoning. \$\endgroup\$
    – James P.
    Aug 5, 2013 at 10:41

1 Answer 1

6
\$\begingroup\$

Why do you check the time with (hours - 12 < 0). This involves two operations, one subtraction and one compare. You could simply use (hours < 12).

Also you rely that in the method set24hrTime(int newTime) the time is never > 23 for your code to work. Why don't you check this?

Also what about the value 0 for your hours? This must be displayed as 12 AM. At the moment you return 0 in this case where it must be 12.

Also for the value 12 of the hours you return 0 as value for the 12hrTime which is wrong.

To sum up I would suggest the following code:

public Clock {
    private int time = 0;
    public int get24hrTime() {
        return time;
    }
    public int set24hrTime(int newTime) {
        if (newTime > 23 || newTime < 0) {
            throw new IllegalArgumentException();
        }
        this.time = newTime;
    }
    public int get12hrTime(){
        if (time == 0) {
            return 12; // Special case 0
        } else if (time <= 12) {
            return time;
        } else {
            return time - 12;
        }
    }
    public String get12hrPostfix() {
        if (time < 12) {
            return "AM";
        } else {
            return "PM";
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Where does the "throw new IlllegalArguemntExeption()" come from and what does it do? \$\endgroup\$
    – Connor
    Feb 1, 2017 at 20:00
  • \$\begingroup\$ The IllegalArgumentException is a RuntimeException of Java. You can throw this exception if you detect an illegal argument (e.g. a negative hour or an hour above 23). \$\endgroup\$
    – Uwe Plonus
    Feb 1, 2017 at 21:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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