4
\$\begingroup\$

I am building a GUI to display a digital clock face (four seven-segment displays), and I have a selector to switch between 12/24-hour formats. I have working code to show both formats, but it is messy. I am looking for a better solution for this section.

The obvious repetition is the section that displays the AM/PM indicator. The only difference is that the fill color of the AM and PM switch.


Note that the time comes in as three integers, one each for hours, minutes, and seconds. These integers are in 24-hour format.

The drawDigits method takes the value to be displayed (0-9, 10=all off), and the (x,y) location of the top-left corner. It then uses a lookup to determine which segments to "turn on."

// 24 hour mode, draw time as reported
if (toggle.isSelected()) {
  drawDigit(hour/10, 20, 100);
  drawDigit(hour%10, 110, 100);
}
// 12 hour mode, display
else {
  if (hour % 12 == 0) {
    drawDigit(1, 20, 100);
    drawDigit(2, 110, 100);
  } else {
    if ((hour % 12)/10 == 1) {
      drawDigit(1, 20, 100);
    } else {
      drawDigit(10, 20, 100);
    }
    drawDigit((hour % 12)%10, 110, 100);
  }
  // show AM/PM indicator
  if (hour >= 12) {
    fill(75);
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
    fill(255, 0, 0);
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
  } else {
    fill(255, 0, 0);
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
    fill(75);
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
  }
}
drawDigit(minute / 10, 224, 100);
drawDigit(minute % 10, 314, 100);
if (second % 2 == 0) fill(240, 0, 0);
else fill(75);
ellipse(200, 145, 10, 10);
ellipse(200, 175, 10, 10);

This code was written in Processing, so the rect, ellipse, text, and fill methods are the ones built-in to Processing.

\$\endgroup\$
  • \$\begingroup\$ If this is in Processing, then why did you use java? \$\endgroup\$ – Jamal Dec 5 '15 at 21:26
  • \$\begingroup\$ Processing is a Java-based language, and you can even use Processing as an external library in native Java code. I marked this as Java because there is not really any Processing-specific code here (the implementation of the built-in functions is irrelevant). \$\endgroup\$ – Eric Roch Dec 6 '15 at 1:43
2
\$\begingroup\$

In general, I think the program is well readable, and demonstrates the idea correctly. I have some suggestions for further improving it.

Constants

Consider the following statement:

drawDigit(hour/10, 20, 100);
drawDigit(hour%10, 110, 100);

Since the values are reused more than once, I would suggest introducing constants for them:

final int HOUR_TO_DIGIT_QUOTIENT = 10;
final int  HOURS_FIRST_X = 20;
final int  HOURS_SECOND_X = 110;
final int  HOURS_Y = 100;

Of course, this holds also for all the other numeric constant, and the strings "AM" and "PM", which are repeated more than once as well.

(Btw., I would suggest introducing these constants even if each value was used only once, because this makes the code easier to understand.)

Organising code

There are some blocks of code, which could be separated into subroutines, in order to make the main code shorter, and thus more readable. E.g.:

fill(75);
rect(40, 230, 10, 10);
text("AM", 55, 240);
fill(255, 0, 0);
rect(40, 245, 10, 10);
text("PM", 55, 255);

This part could be extracted to methods, e.g. like this:

void showAmIndicactor(boolean enabled) {
    if (enabled) {
        fill(255, 0, 0);
    }
    else {
        fill(75);
    }
    rect(40, 230, 10, 10);
    text("AM", 55, 240);
}

void showPmIndicactor(boolean enabled) {
    if (enabled) {
        fill(255, 0, 0);
    }
    else {
        fill(75);
    }
    rect(40, 245, 10, 10);
    text("PM", 55, 255);
}


// in the main block:
if (hour >= 12) {
   showAmIndicactor(false);
   showPmIndicactor(true);
 } else {
   showAmIndicactor(true);
   showPmIndicactor(false);
 }

N.B.: I am not familiar with Processing in detail, but since from the question I understand that 'drawDigit' is an own method (i.e. not built in to Processing), I assume, that it is possible to introduce other own methods in a similar way as in Java.

Functionality

I do not understand, how the seconds will be rendered correctly. As far as I see, this is the only part that depends on the seconds:

if (second % 2 == 0) fill(240, 0, 0);
else fill(75);

But as I understand, this code does not show any digits, just fills a space with a colour. Is this the intended behaviour? If so, then I suggest documenting why is it like that (in a comment), otherwise, I propose to extend the functionality so that seconds are rendered as well.

On a side note, I recommend to use always the block-form of if-else statement (and any other statements), even if it has just one instruction per branch. In other words, I suggest rewriting it as follows:

if (second % 2 == 0) {
     fill(240, 0, 0);
}
else {
     fill(75);
}

This prevents that you add more statements (accidentally indented at the level of if-else block), and think they will be executed within if-else.

Validation

Although not part of the original problem statement, I would recommend to extend your code with validation. I.e., make sure that hours are between 0-23 (or 1-12 in case of 12-hour format), minutes and seconds are between 0-59, and throw an exception otherwise (or print an error and exit, whichever is available in Processing).

Before adding validation, it would be also interesting to experiment how your program behaves with invalid input (e.g. negative numbers, hours consisting of three digits or four, etc.)

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your feedback. I have incorporated what you suggested and posted my entire project on CR if you're interested. Check it out here \$\endgroup\$ – Eric Roch Dec 9 '15 at 2:43

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.