patternjavaModerate
Time to Next Departure
Viewed 0 times
nextdeparturetime
Problem
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
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
```
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 5
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 5
Solution
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
This way, it will be ok even if
Remove the duplication
In every
Simplify calculations
You're setting
Since
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:
This transformation helps with the
Second, the
Consider this implementation:
Note that the
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:
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:
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));
a
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 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 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 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));
a
Code Snippets
switch (findNextDeparture % 60) {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;
}int initialTime = (int) (time / (1000 * 60) % 60);
int findNextDeparture = initialTime;int findMinutesToNextDeparture(int initialTime) { ... }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;
}Context
StackExchange Code Review Q#63055, answer score: 14
Revisions (0)
No revisions yet.