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 have two classes named Date.class and an enum class Time. I have declared these class like this:

public enum Time{
DAY,NIGHT
}

And the Date class is like this:

public class Date{
public boolean isDay(){
        Calendar timeNow =Calendar.getInstance();
        int hour =timeNow.get(Calendar.HOUR_OF_DAY);
        if (START_OF_DAY<=hour && hour<=END_OF_DAY){
            return true;
        }
        return false;
    }
}

The isDay() method returns true if the time is in between a range.

In the main class I am trying to set icons like this:

if(date.isDay()){
setIcon(Time.DAY);
}else {
setIcon(Time.NIGHT);
}

private void setIcon(Time time) {
        if (time==Time.DAY){
           setDayImage();
        }else if(time==Time.NIGHT){
            setNightImage();
        }
    }

 private void setNightImage() {
        if (nightImage==null)
           nightImage.setImage();
    }

    private void setDayImage() {
       if (dayImage==null)
           dayImage.setImage();
    }

How can I improve the design of this code?

share|improve this question

closed as off-topic by JaDogg, Marc-Andre, Kid Diamond, vnp, Schism Aug 27 at 20:31

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – JaDogg, Marc-Andre, Kid Diamond, vnp, Schism
If this question can be reworded to fit the rules in the help center, please edit the question.

1  
One thing that may help is inline if statements (I believe the name is ternary operators, but I'm not positive on that). For example setIcon(date.isDay() ? Time.DAY : Time.NIGHT);. They basically work as statement ? ifTrue : isFalse –  Tom Hart Aug 27 at 14:09
3  
Serious bug: if nightImage is null, how can you call nightImage.setImage()??? –  h.j.k. Aug 27 at 16:17
    
@h.j.k. is correct code is broken, voted to close –  JaDogg Aug 27 at 18:38

1 Answer 1

up vote 6 down vote accepted

unnecessary code

You basically check if it is day twice. Once here:

if(date.isDay()){
setIcon(Time.DAY);
}else {
setIcon(Time.NIGHT);
}

and then again in your setIcon function. This is redundant. There are many ways to fix this, my preferred way would be this:

change boolean isDay() to Time getTime(), then return the Time enum. Then just call setIcon(date.getTime()).

Indentation and Spacing

  • you sometimes use 4 spaces to intend, and sometimes 8 (and sometimes even 0 or 1). This should be uniform.
  • it's customary to put spaces between a closing ) and the following character (eg an opening {), as well as between comparisons (<=, ==, etc).

This makes code hard to read, but luckily you can use any modern IDE to fix this easily.

if-else return true-false

    if (START_OF_DAY<=hour && hour<=END_OF_DAY){
        return true;
    }
    return false;

can be written as

return START_OF_DAY <= hour && hour <= END_OF_DAY;
share|improve this answer

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