2
\$\begingroup\$

Anyone know how I write this code shorter, with modulos maybe? I also want to add weeks and years do this code, but it kind of got too long for that. Maybe there is some smart algoritm? Something in a for-loop?

public static String formatSeconds (long seconds)
{
    if (seconds < 60)
        return seconds != 1 ? seconds + " seconds" : "1 second";

    if (seconds < 3600)
    {
        seconds /= 60;
        return seconds != 1 ? seconds + " minutes" : "1 minute";
    }

    if (seconds < 86400)
    {
        seconds /= 3600;
        return seconds != 1 ? seconds + " hours" : "1 hour";
    }

    if (seconds < 2629743)  //month
    {
        seconds /= 86400;
        return seconds != 1 ? seconds + " days" : "1 day";
    }

    seconds /= 2629743;
    return seconds != 1 ? seconds + " months" : "1 month";
}
\$\endgroup\$
2
\$\begingroup\$

This is a case for an Enum and a NavigableMap.

TimeUnit.java

public enum TimeUnit {

    SECOND(1, "1 second", " seconds"),
    MINUTE(60, "1 minute", " minutes"),
    HOUR(3600, "1 hour", " hours"),
    DAY(86400, "1 day", " days"),
    MONTH(2629743, "1 month", " months");

    private final int SECONDS_COUNT;
    private final String ONE_UNIT;
    private final String MULTIPLE_SUFFIX;

    public TimeUnit(int secondsCount, String oneUnit, String multipleSuffix) {
        SECONDS_COUNT = secondsCount;
        ONE_UNIT = oneUnit;
        MULTIPLE_SUFFIX = multipleSuffix;
    }

    public getSecondsCount() {
        return SECONDS_COUNT;
    }

    public getOneUnit() {
        return ONE_UNIT;
    }

    public getMultipleSuffix() {
        return MULTIPLE_SUFFIX;
    }

}

TimeProcessor.java

public class TimeProcessor {

    private static final NavigableMap<Integer, TimeUnit> timePeriods = new TreeMap<>();
    static {
        for (TimeUnit unit : TimeUnit.values()) {
            timePeriods.put(unit.getSecondsCount(), unit);
        }
    }

    public static String toString(int secondsOfTime) {
        Map.Entry<Integer, TimeUnit> entry = timePeriods.floorEntry(secondsOfTime);
        if (entry == null || entry.getKey() == null) {
            return "";
        }

        int unitCount = secondsOfTime / entry.getKey();
        return toString(unitCount, entry.getValue());
    }

    public static String toString(int unitCount, TimeUnit unit) {
        return (unitCount == 1) ? unit.getOneUnit() : unitCount + unit.getMultipleSuffix();
    }

}

I find it easier to have all the data localized in one place, separate from the handling logic. This makes it easier and more robust to add additional units like weeks, fortnights, seasons, years, decades, centuries, and millennia.

This is more verbose and self-documenting as well as easier to extend and maintain. Each new unit is only a line of code.

When using if/else structures or ternary operators, I prefer to test positive conditions rather than negatives. This avoids the inherent if not not logic.

When using either this version or your original, there is a bit of a problem in terms of how it handles months. Not every month has the same length. A month can be anywhere from twenty-eight to thirty-one days. It might be better to make use of the existing date handling logic rather than spin your own. Because months and larger units are not fixed length. Even days can be problematic with daylight savings time. I regard this more as a programming exercise than a solution to representing time intervals.

I haven't tested this, so beware of syntax or logical errors.

\$\endgroup\$

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.