Time to Next Departure

Question

I am trying to find out how many minutes that are remaining until the next train will departure, based on the current system time in Java.

I am so far not familiar with the many aspects of Java, but I have managed to get the program to work in most use cases with a switch statement and while loop, but it feels like I am using too many workarounds to get the code to work as intended.

The next step is that I am trying to complete, the task in a less tedious and more elegant way. I was wondering if anyone could point me in the right direction to how the task could be completed in a similar way but with other features of Java, such as ArrayLists, tables etc. that are suitable for what I am trying to accomplish.

public class TrainDepartures{

public static void main(String[] args) {

    long time = System.currentTimeMillis();
    int initialTime = (int) (time / (1000 * 60) % 60);
    int findNextDeparture = (int) (time / (1000 * 60) % 60);

    boolean foundDeparture = false;

    loop: while (foundDeparture != true) {
        findNextDeparture++;
        switch (findNextDeparture) {
            case 10:
                 System.out.print("Departure in " + (Math.abs(initialTime - 10)) + " min.");
                break loop;
            case 20:
                System.out.print("Departure in " + (Math.abs(initialTime - 20)) + " min.");     
                break loop;
            case 30:
                System.out.print("Departure in " + (Math.abs(initialTime - 30)) + " min.");     
                break loop;
            case 40:
                System.out.print("Departure in " + (Math.abs(initialTime - 40)) + " min.");     
                break loop;
            case 50:
                System.out.print("Departure in " + (Math.abs(initialTime - 50)) + " min.");     
                break loop;
            case 52:
                System.out.print("Departure in " + (Math.abs(initialTime - 52)) + " min.");     
                break loop;
            case 55:
                System.out.print("Departure in " + (Math.abs(initialTime - 55)) + " min.");     
                break loop;
            case 56:
                System.out.print("Departure in " + (Math.abs(initialTime - 56)) + " min.");     
                break loop;
            case 57:
                System.out.print("Departure in " + (Math.abs(initialTime - 57)) + " min.");     
                break loop;         
}
}
}
}

Answer 1

You have a bug

Don't run this code when the current time is 58 or 59 minutes. The loop will never end. You can fix that by adding a modulo in the switch statement:

switch (findNextDeparture % 60) {

This way, it will be ok even if findNextDeparture overflows, you will still get the correct time until the next departure. For example starting from 58, 70 % 60 will match case 10:, giving you 70 - 58 = 12 minutes.

Remove the duplication

In every case statement you do essentially the same thing. You could reduce that to this:

switch (findNextDeparture) {
    case 10:
    case 20:
    case 30:
    case 40:
    case 50:
    case 52:
    case 55:
    case 56:
    case 57:
        System.out.print("Departure in " + Math.abs(initialTime - findNextDeparture) + " min.");     
        break loop;
}

Simplify calculations

You're setting findNextDeparture to the same value as initialTime. Instead of spelling out that long expression, it would be better to reuse it:

int initialTime = (int) (time / (1000 * 60) % 60);
int findNextDeparture = initialTime;

---

Since findNextDeparture starts from the same value as initialTime, and it always increases, you don't need Math.abs(initialTime - findNextDeparture), you can do simply findNextDeparture - initialTime for the same effect.

Improving the implementation

First of all, as others have suggested, the main logic of finding the next departure date should be extracted to its own method, and return the minutes to next departure instead of printing it. Consider this signature:

int findMinutesToNextDeparture(int initialTime) { ... }

This transformation helps with the while loop too: instead of using the loop label, you could just return. Simpler, and more natural, because labels are not a usual practice in Java, and it probably indicates code smell. This transformation helps with another thing: it makes the method testable, see more on that later.

Second, the while loop, the switch, the list of case statements... Not cool! For one thing, stepping through values one by one, sounds silly, doesn't it. If the values are in a list, you could make bigger jumps than doing it 1 by 1. But there's something even better: binary search!

Consider this implementation:

public static int findMinutesToNextDeparture(int initialTime) {
    List<Integer> times = Arrays.asList(10, 20, 30, 40, 50, 52, 55, 56, 57);
    // index of the search key, if it is contained in the array;
    // otherwise, (-(insertion point) - 1).
    // The insertion point is defined as the point at which
    // the key would be inserted into the array:
    // the index of the first element greater than the key, or a.length
    // if all elements in the array are less than the specified key.
    // Note that this guarantees that the return value will be >= 0
    // if and only if the key is found.
    int index = Collections.binarySearch(times, initialTime);
    final int nextDeparture;
    if (-index > times.size()) {
        nextDeparture = times.get(0) + 60;
    } else if (index < 0) {
        nextDeparture = times.get(-index - 1);
    } else {
        nextDeparture = times.get(index + 1);
    }
    return nextDeparture - initialTime;
}

Note that the times list would be probably better as a parameter. I left it inside the method just for the sake of this example. Now, the logic looks quite tricky. I included the JavaDoc of Collections.binarySearch for reference and because it's kind of hopeless to remember this stuff.

Keep in mind that the list must be sorted for binary search to work. If you make a mistake when typing the elements like this it, it will be broken:

List<Integer> times = Arrays.asList(10, 30, 20, 40, 50, 52, 55, 56, 57);

To make sure you cannot make mistakes, you can wrap the list inside a sorted collection, and then again in a random access list like this:

List<Integer> times = new ArrayList<>(new TreeSet<>(Arrays.asList(10, 30, 20, 40, 50, 52, 55, 56, 57)));

Unit testing

I'll be honest: getting the binary search technique right was not easy. It helped that I already had unit tests for the original working implementation. Before you refactor something, make sure you have solid unit tests. This way you can protect yourself from accidentally breaking your working code.

@Test
public void testMinutesBeforeFirst() {
    assertEquals(10, TrainDepartures.findMinutesToNextDeparture(0));
    assertEquals(7, TrainDepartures.findMinutesToNextDeparture(3));
}

@Test
public void testMinutesInBetween() {
    assertEquals(7, TrainDepartures.findMinutesToNextDeparture(13));
    assertEquals(7, TrainDepartures.findMinutesToNextDeparture(43));
}

@Test
public void testExactMatches() {
    assertEquals(2, TrainDepartures.findMinutesToNextDeparture(50));
    assertEquals(3, TrainDepartures.findMinutesToNextDeparture(52));
}

@Test
public void testMinutesBeyondLast() {
    assertEquals(12, TrainDepartures.findMinutesToNextDeparture(58));
    assertEquals(11, TrainDepartures.findMinutesToNextDeparture(59));
    assertEquals(9, TrainDepartures.findMinutesToNextDeparture(61));
}

Answer 2

Lets take a look at the first two cases of the switch statement.

case 10:
     System.out.print("Departure in " + (Math.abs(initialTime - 10)) + " min.");
    break loop;
case 20:
    System.out.print("Departure in " + (Math.abs(initialTime - 20)) + " min.");     
    break loop;

These follows the same pattern.

case XXX:
    System.out.print("Departure in " + (Math.abs(initialTime - XX)) + " min.");     
    break loop;

The XXX is the only difference that appears in those two cases. If you look at the rest of the cases, the same is true. This is a clear case of where you want to extract the repeated code.

double calculateNextDeparture(int initialTime, int xxx) {
  return Math.abs(initialTime - xxx);
}

Now you can use this function to calculate the answer for any xxx value. It should be noted that xxx is not a very good variable name. I left it as such because it is not entirely clear what makes the specific values of each case statement special. You should replace xxx with a name that describes what the value is meant to represent.

Now that we have a better way to calculate the answer, we need a better way to decide when this calculation needs to occur. Using a switch statement does not scale well. What happens if you wanted to produce a value for every number between 1 and 60? Would you have 60 cases?

A better idea is to create of list of values that you want to produce and answer for. Then if the current value is in the list, print the result.

List<int> matchValues = Arrays.asList(10, 20, 30);
while (foundDeparture != true) {
  findNextDeparture++;
  if (matchValues.contains(findNextDeparture)) {
    System.out.print("Departure in " + calculateNextDeparture(initialTime, findNextDeparture)) + " min.");
  }
}

This provides the same functionality and you only have to set the key values in one place.

---

Additional points:

while (foundDeparture != true) 

is equivalent to

while (!foundDeparture)

Since it is already a boolean value, you don't need to explicitly compare it to true.

---

loop: while (foundDeparture != true) {
    findNextDeparture++;
    switch (findNextDeparture) {
        case 10:
             System.out.print("Departure in " + (Math.abs(initialTime - 10)) + " min.");
            break loop;
    }
}

Since you don't have anything after the switch statement, breaking to the label does not do anything. The while loop will ensure that the code returns to the beginning of the loop one the whole block has completed.

---

Nothing ever sets foundDeparture to true. This means your loop will never finish.

---

The calculation done for initialTime and findNextDeparture is the same. Additionally, since it is just a mathematical operation, it is hard to know what it is doing. Extracting it into a well named function would remove repeated code and tell people who don't know the code what operation is happening.

---

findNextDeparture is not a good variable name. The name conveys the idea that some operation is being performed. However, this is just a value that changes during the loop. The variable name should describe what the variable is meant to contain.

Answer 3

The main problem is that you're mixing output with printing. This makes you program single-shot only and even complicates the design. I may write more later, but the most important thing is: Separation of concerns.

Write a method doing input, write the working method, write a method doing output.

---

Actually, you program badly needs an input. Imagine someone wants to know what you program says in half an hour.

If you're using an IDE, there's a feature called "extract method".

---

You may thing that writing a monolithic block is the easiest, but it's not Imagine something like

private static final long MILLIS_PER_MINUTE = 60L * 1000L;
private static final long MILLIS_PER_HOUR = 60L * MILLIS_PER_MINUTE;

private long pastFullHour(long millis) {
    return millis / MILLIS_PER_HOUR * MILLIS_PER_HOUR;
}

private long nextFullHour(long millis) {
    return pastFullHour(millis) + MILLIS_PER_HOUR;
}

private long millisToNextFullHour(long millis) {
    return nextFullHour(millis) - millis;
}

private long minutesToNextFullHour(long millis) {
    return millisToNextFullHour(millis) / MILLIS_PER_MINUTE;
}

I haven't tested it, but it's be fairly easy to do. Each method does one pretty trivial thing. If anything goes wrong, you know that either the tested method is wrong, or one of those it uses. This makes it trivial.

You may say that you get too many methods this way, and that's true, but you can inline them later (and the IDE can do this for you).