4
\$\begingroup\$

I have a time, in seconds, which has the possibility to be very large. I wish to convert said time into the "appropriate" rounded, readable format.

I already have code which achieves this, however it's not very efficient (and contains a whole bunch of magic numbers):

String readable = decayTime + " minutes";
if(decayTime > 60)
{
    decayTime /= 60;
    readable = decayTime + " hours";

    if(decayTime > 24)
    {
        decayTime /= 24;
        readable = decayTime + " days";

        if(decayTime > 365)
        {
            decayTime /= 365;
            readable = decayTime + " years";

            if(decayTime > 1000000)
            {
                decayTime /= 1000000;
                readable = decayTime + "mn years";

                if(decayTime > 1000)
                {
                    decayTime /= 1000;
                    readable = decayTime + "bn years"; 
                }
            }
        }
    }

Apart from switching out the magic numbers, I can't personally think how to make it better.

What would be a better approach to this, or is there something in-built which could help?

\$\endgroup\$
1
  • \$\begingroup\$ Just nitpicking: The numbers you are using in your code are in no way magic. They are numbers, albeit obvious ones, since they represent well-established time units. \$\endgroup\$ – Roland Illig Aug 30 '16 at 23:36
3
\$\begingroup\$

Here's one approach, using TreeMap. It looks up your number of milliseconds in a pre-populated map, finds the appropriate entry and does the division.

Just create one of these objects, and call the format method as many times as you need to.

Note that it's not quite right for negative arguments to format - but you might want to put your own logic in around that, for example, to throw some kind of exception.

import java.util.Map;
import java.util.NavigableMap;
import java.util.TreeMap;

public class TimeFormatter {

    private NavigableMap<Long,String> timeUnits = new TreeMap<>();

    public TimeFormatter() {
        timeUnits.put(Long.MIN_VALUE, " is not a valid argument");
        timeUnits.put(1L, " milliseconds");
        timeUnits.put(1000L, " seconds");
        timeUnits.put(60 * 1000L, " minutes");
        timeUnits.put(60 * 60 * 1000L, " hours");
        timeUnits.put(24 * 60 * 60 * 1000L, " days");
        timeUnits.put(365 * 24 * 60 * 60 * 1000L, " years");
        timeUnits.put(1000000L * 365 * 24 * 60 * 60 * 1000L, " million years");
        timeUnits.put(1000000000L * 365 * 24 * 60 * 60 * 1000L, " billion years");
    }

    public String format(long milliseconds) {
        Map.Entry<Long,String> unitBelow = timeUnits.floorEntry(milliseconds);
        return milliseconds / unitBelow.getKey() + unitBelow.getValue();
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ That's pretty neat! I never knew about TreeMap and that definitely looks a lot neater than my code. \$\endgroup\$ – Jake Stanger Aug 29 '16 at 23:32
  • \$\begingroup\$ There's a TimeUnit class that can be used instead of calculating ms values for each period \$\endgroup\$ – krp Aug 30 '16 at 6:52
  • \$\begingroup\$ Nice use of TreeMap. You will get off-by-somes for all the leap years below the 1M :) \$\endgroup\$ – RobAu Aug 30 '16 at 11:29
  • \$\begingroup\$ @RobAu Yeah, I know. I figured that dealing with leap years was a whole other problem; and one that didn't figure in the OP's original code. \$\endgroup\$ – Dawood ibn Kareem Aug 30 '16 at 12:45
  • \$\begingroup\$ Daylight savings is also a whole other problem that I didn't bother dealing with. \$\endgroup\$ – Dawood ibn Kareem Aug 30 '16 at 12:48
2
\$\begingroup\$

How many days in a year?

    if(decayTime > 365)
    {
        decayTime /= 365;
        readable = decayTime + " years";

This isn't quite correct. There are closer to 365.2425 days in a year. Don't forget leap years and leap centuries. And on the scale of millions and billions of years, you can expect leap seconds.

Make constants static

If you use a NavigableMap, you should make it static final, as it will have the same values for every instance of the class.

Consider

public class TimeFormatter {

    private static final NavigableMap<Long,String> TIME_UNITS = new TreeMap<>();

    static {
        TIME_UNITS.put(Long.MIN_VALUE, " is not a valid argument");
        TIME_UNITS.put(TimeUnit.SECONDS.toSeconds(1), " seconds");
        TIME_UNITS.put(TimeUnit.MINUTES.toSeconds(1), " minutes");
        TIME_UNITS.put(TimeUnit.HOURS.toSeconds(1), " hours");
        TIME_UNITS.put(TimeUnit.DAYS.toSeconds(1), " days");
        TIME_UNITS.put(TimeUnit.DAYS.toSeconds(1) * 3652425 / 10000, " years");
        TIME_UNITS.put(TimeUnit.DAYS.toSeconds(100L * 3652425), " million years");
        TIME_UNITS.put(TimeUnit.DAYS.toSeconds(100000L * 3652425), " billion years");
    }

I also put the name in ALL_CAPS, as that is a common Java convention for constants.

I used a static initializer block.

I did not try to include leap seconds.

It's not clear to me why days and years but not weeks, months, decades, centuries, and millennia. Fortnights are a reasonable exclusion, but the others are pretty standard.

\$\endgroup\$
4
  • \$\begingroup\$ I didn't mention because I didn't really think it would be particularly important, but this is for a game mod, so the day/night cycle is a fixed period of time (and dead accuracy isn't important, it's mostly for show...). Months require more work than they're worth (the number of days vary) and I figured people are capable of interpreting "234 days" or whatever. The latter point also applies to decades/centuries/millenia. I will bear the variable comments in mind though. \$\endgroup\$ – Jake Stanger Aug 30 '16 at 23:39
  • \$\begingroup\$ No, there are NOT 365.2425 days in a year. There are 365 days in some years, and 366 days in others. To be able to deal with this "correctly", we'd need more information than just the number of milliseconds. For example, for 730 days worth of milliseconds, this could be expected to output "1 year", because it's less than two years if one is a leap year; or it could be expected to output "2 years" if both years happen to be normal years. Therefore, we'd need a method which takes both the number of milliseconds and a "start date" as parameters. But OP doesn't want this. The question ... \$\endgroup\$ – Dawood ibn Kareem Aug 31 '16 at 1:44
  • \$\begingroup\$ ... shows calculations involving only the number of milliseconds - so this level of accuracy is impossible. In particular, your code can display "365 days" with the right argument. It seems clear from OP's original code that that's not what's required. \$\endgroup\$ – Dawood ibn Kareem Aug 31 '16 at 1:45
  • \$\begingroup\$ @DavidWallace In every four hundred year period, regardless of start date, there are exactly 146,097 days. That includes 97 leap days. In a scope measuring millions and billions of years, this will produce an error of just under a year for every 1500 years. That's an error of over 600 years at a million. And I'm not sure why you feel that consistently overestimating the number of years is better than estimating on average. Any eight year or more period will be wrong in the original method, as there is always at least one leap year in any eight year period. \$\endgroup\$ – mdfst13 Aug 31 '16 at 6:52
0
\$\begingroup\$

Here is a quick edit to Daivd Wallace's answer to incorporate TimeUnit as @krp suggested.

import java.util.Map;
import java.util.NavigableMap;
import java.util.TreeMap;
import java.util.concurrent.TimeUnit;

public class TimeFormatter {

    private NavigableMap<Long,String> timeUnits = new TreeMap<>();

    public TimeFormatter() {
        timeUnits.put(Long.MIN_VALUE, " is not a valid argument");
        timeUnits.put(TimeUnit.SECONDS.toSeconds(1), " seconds");
        timeUnits.put(TimeUnit.MINUTES.toSeconds(1), " minutes");
        timeUnits.put(TimeUnit.HOURS.toSeconds(1), " hours");
        timeUnits.put(TimeUnit.DAYS.toSeconds(1), " days");
        timeUnits.put(TimeUnit.DAYS.toSeconds(365), " years");
        timeUnits.put(TimeUnit.DAYS.toSeconds(365 * 1000000L), " million years");
        timeUnits.put(TimeUnit.DAYS.toSeconds(365 * 1000000L * 1000), " billion years");
    }

    public String format(long milliseconds) {
        Map.Entry<Long,String> unitBelow = timeUnits.floorEntry(milliseconds);

        int time = (int) (milliseconds / unitBelow.getKey());
        String formatted = time + unitBelow.getValue();

        if(time == 1 && unitBelow.getKey() < TimeUnit.DAYS.toSeconds(365 * 1000000L)) formatted = formatted.substring(0, formatted.length()-1); //Remove plural

        return formatted;
    }
}

This version also handles plurals by removing the trailing 's'.

It's not perfect as TimeUnit only scales to days, but it removes a lot of the magic numbers.

\$\endgroup\$
4
  • \$\begingroup\$ I'm afraid, that an input of 60 * 60 * 24 * 365 * 1000000 (1.000.000 years) would yield an output of 1 million year - with the s missing in this case. Million and billion suggest a plural form. ideone. This is just a minor annoyance, I guess. \$\endgroup\$ – vonludi Aug 30 '16 at 13:33
  • \$\begingroup\$ @ideone true, I didn't think of that. It would be a pretty simple fix with an if statement; I'll post an update in a minute... \$\endgroup\$ – Jake Stanger Aug 30 '16 at 18:18
  • \$\begingroup\$ I think your fix is wrong. I can still get "1 years" from this code. \$\endgroup\$ – Dawood ibn Kareem Aug 31 '16 at 1:37
  • \$\begingroup\$ I think I know why. I just stuck in unitBelow.getKey() < 1M but the key is the number of seconds. I think TimeUnit.DAYS.toSeconds(365 * 1M) should work. \$\endgroup\$ – Jake Stanger Sep 3 '16 at 15:01

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.