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.

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";
        }
    }
}
share|improve this question
1  
"Do not fix something that works", what is the issue with this code? –  Adam Siemion Aug 5 '13 at 10:38
1  
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. –  James Poulson Aug 5 '13 at 10:41

1 Answer 1

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";
        }
    }
}
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.