Skip to main content
Code Review

Return to Answer

added 166 characters in body
Source Link
Jerry Coffin
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143
// In real code, you'll need to pick some class that implements the Map interface,
// but for the moment, I'm not really concerned with which one you pick.
Map<Integer, string> names;

names.put(360, "6 minutes left");
names.put(300, "5 minutes left");
//...
names.put(5, "5 seconds left");

Map<Integer, string> names;

names.put(360, "6 minutes left");
names.put(300, "5 minutes left");
//...
names.put(5, "5 seconds left");

// In real code, you'll need to pick some class that implements the Map interface,
// but for the moment, I'm not really concerned with which one you pick.
Map<Integer, string> names;

names.put(360, "6 minutes left");
names.put(300, "5 minutes left");
//...
names.put(5, "5 seconds left");
added 163 characters in body
Source Link
Jerry Coffin
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143

When an else follows a chain of if statements like this, it attaches to the most recent if that doesn't already have an else. An if statement controls exactly one following statement (which needs to be a compound statement if you want it to control more than one syntactical statement). In other words, the code really works like:

Then the code just looks up the current time in names and prints out a string if there is one for a particular value. Aside: while I've used names here, it's probably not a good example to emulate--when naming the real variable, you should almost certainly pick a name that better reflects its intended function; something like timeToString, for example.

When an else follows a chain of if statements like this, it attaches to the most recent if that doesn't already have an else. In other words, the code really works like:

Then the code just looks up the current time in names and prints out a string if there is one for a particular value.

When an else follows a chain of if statements like this, it attaches to the most recent if that doesn't already have an else. An if statement controls exactly one following statement (which needs to be a compound statement if you want it to control more than one syntactical statement). In other words, the code really works like:

Then the code just looks up the current time in names and prints out a string if there is one for a particular value. Aside: while I've used names here, it's probably not a good example to emulate--when naming the real variable, you should almost certainly pick a name that better reflects its intended function; something like timeToString, for example.

Source Link
Jerry Coffin
Jerry Coffin
  • 33.6k
  • 4
  • 75
  • 143

First: I think the point @DFord makes is quite good.

Second, I think you should work on indenting your code better. In particular, the indentation of your main if/then/else chain is misleading:

        if (endTime - startTime > 1)
            if (endTime - startTime == 360)
                System.out.println("6 Minutes left");
            if (endTime - startTime == 300)
                System.out.println("5 Minutes left");
            if (endTime - startTime == 240)
                System.out.println("4 Minutes left");
            if (endTime - startTime == 180)
                System.out.println("3 Minutes left");
            if (endTime - startTime == 120)
                System.out.println("2 Minutes left");
            if (endTime - startTime == 60)
                System.out.println("1 Minute left");
            if (endTime - startTime == 30)
                System.out.println("30 Seconds left");
            if (endTime - startTime == 15)
                System.out.println("15 Seconds left");
            if (endTime - startTime == 5)
                System.out.println("5 Seconds left");
        else if (endTime - startTime == 1) {
            System.out.println("Foo is up!");

When an else follows a chain of if statements like this, it attaches to the most recent if that doesn't already have an else. In other words, the code really works like:

if (endTime - startTime > 1)
    if (endTime - startTime == 360)
        System.out.println("6 Minutes left");

// The following are evaluated regardless of whether the `> 1` condition was met
if (endTime - startTime == 300)
    System.out.println("5 Minutes left");
if (endTime - startTime == 240)
    System.out.println("4 Minutes left");
if (endTime - startTime == 180)
    System.out.println("3 Minutes left");
if (endTime - startTime == 120)
    System.out.println("2 Minutes left");
if (endTime - startTime == 60)
    System.out.println("1 Minute left");
if (endTime - startTime == 30)
    System.out.println("30 Seconds left");
if (endTime - startTime == 15)
    System.out.println("15 Seconds left");
if (endTime - startTime == 5)
    System.out.println("5 Seconds left");
else if (endTime - startTime == 1) {
    System.out.println("Foo is up!");

Based on your indentation, you apparently wanted it to be more like this:

if (endTime - startTime > 1) {
    if (endTime - startTime == 360)
        System.out.println("6 Minutes left");
    if (endTime - startTime == 300)
        System.out.println("5 Minutes left");
    if (endTime - startTime == 240)
        System.out.println("4 Minutes left");
    if (endTime - startTime == 180)
        System.out.println("3 Minutes left");
    if (endTime - startTime == 120)
        System.out.println("2 Minutes left");
    if (endTime - startTime == 60)
        System.out.println("1 Minute left");
    if (endTime - startTime == 30)
        System.out.println("30 Seconds left");
    if (endTime - startTime == 15)
        System.out.println("15 Seconds left");
    if (endTime - startTime == 5)
        System.out.println("5 Seconds left");
}
else if (endTime - startTime == 1) {
    System.out.println("Foo is up!");

In this case, the braces prevent the else from attaching to the immediately previous if, forcing it to attach to the if you apparently intended instead.

Although it might initially seem (or even be) somewhat wasteful, I'd tend to avoid the issue entirely: instead of a cascade of if statements, I'd probably put the statements to be printed out into some kind of Map, then just do a lookup in the map and print out the associated value (if it has one) for a given time.

Map<Integer, string> names;

names.put(360, "6 minutes left");
names.put(300, "5 minutes left");
//...
names.put(5, "5 seconds left");

Then the code just looks up the current time in names and prints out a string if there is one for a particular value.

Also note that this makes it fairly easy to export the values and strings to an configuration file instead of them being embedded in the code. If, for example, you ended up wanting to translate the program to Russian or French (or whatever) translating these strings wouldn't (in and of itself) force re-compiling the associated code. Likewise, if you decided to add or remove some of the messages, moving them to an external data file makes this trivial to do.

That said, I should probably add that I doubt you could localize any significant program without re-compiling at all (or without rewriting at least a little of the code). Nonetheless, keeping strings like this external to the code does help keep the task a little more manageable.