Lap time formatting code is confusing

I have this code from school and I can't seem to understand exactly what is it doing. I know it shows a lap time, like those from sports, converted in a minutes, seconds and milliseconds, but I don't understand what the 2 string variable are and what they do.

Here's the code:

public String getTimeFormat(long ms){
    String s1 = ""+ms;
    Date date = null;
    try {
        date = new SimpleDateFormat("SSS").parse(s1);
    } catch (ParseException e){
        e.printStackTrace();
    }
    String s2 = new SimpleDateFormat("mm:ss:SS").format(date);
    return s2;
}
Jon Skeet
people
quotationmark

Firstly, this is really bad code in my view.

It's converting the original ms value into a string (so 35968 would become "35968") and then parsing that into a Date, as if from a format which only specifies a number of milliseconds... which is then interpreted as "the number of milliseconds since the Unix epoch in the formatter's time zone".

It's then converting that Date value into a minutes/seconds/milliseconds format by formatting it with another SimpleDateFormat. The milliseconds are only formatted to 2 decimal places, although it's odd to use : as a separator here rather than .. (It makes it look like an hours/minutes/seconds value, rather than minutes/seconds/milliseconds.)

Both formats use the same time zone, which at least sort of avoids problems there. However:

  • If the system default time zone had transitions around the Unix epoch, there could still be some oddities
  • Fundamentally it's trying to format a duration by treating it as a date and time. That's a bad idea
  • The exception "handling" is laughable - if an exception is thrown when parsing, the first exception will then be printed and a NullPointerException will be thrown by the following line, as date will still be null
  • If the duration is more than an hour, the information will silently be lost; it's not clear what the desired behaviour is here
  • Even if you did want to format it this way, it would be simpler to use new Date(ms) and then format it with a SimpleDateFormat with the time zone set to UTC. There's no need for any string parsing here

I can't easily provide better code without knowing a few more requirements, but if you were trying to replace this code you should consider:

  • Given that you have milliseconds, do you definitely only want tens-of-milliseconds precision in the display?
  • What do you want to do if ms is negative?
  • What do you want to do if ms represents more than an hour?
  • Do you definitely want : as the separator between seconds and milliseconds?

You might then want to use String.format to format the value. Unfortunately neither Joda Time nor java.time in Java 8 has a good way of performing custom Duration formatting, which is what you really want here.

people

See more on this question at Stackoverflow