7
\$\begingroup\$

This is a fun piece. I have this block of code here:

if (minutes >= 60 && hours < 24) {
        return hours + " hours ago";
    } else if (hours >= 24 && days <= 30) {
        return days + " days ago";
    } else if (minutes <= -60 && hours > -24) {
        return -hours + " hours from now";
    } else if (hours <= -24 && days >= -30) {
        return -days + " days from now";
    } else if (minutes >= -60 && minutes < 0) {
        return -minutes + " minutes from now";
    } else if (days > 30 && days < 365) {
        if (days/30 == 1) {
            return "Last month";
        }
        return days/30 + " months ago";
    } else if (days < -30 && days > -365) {
        if (-days/30 == 1) {
            return "Next month";
        }
        return -(days/30) + " months from now";
    } else if (days >= 365) {
        if (days/365 == 1) {
            return "Last year";
        }
        return days/365 + " years ago";
    } else if (days <= -365) {
        if (-days/365 == 1) {
            return "Next year";
        }
        return -(days/365) + " years from now";
    } else {
        return minutes + " minutes ago";
    }

I want to reduce the number of if-else statements. The ways I was thinking of tackling this was:

  1. Store these into a hashmap and then call it \$O(1)\$ time
  2. Use enums
  3. Ternary operators
  4. Switch statements

I don't want to use enums since this is on a mobile device, and enums are heavier on memory. I was thinking that maybe ternary operators are best here. Thoughts?

The negative is intentional. It's with an API that I am working with and they use negatives to imply future events.

Input from API:

This is the following example input from the API:

"time": "2015-03-06 20:00:00 EST"

It is passed to the following code block, and it is expected to return something like:

"45 minutes ago"
"56 seconds ago"
"1 hour from now"
"2 days from now"
"2 months from now"
"Next year"
"Two years from now"
"A month ago"
"28 days ago"
"5 mins from now"
//etc.
\$\endgroup\$
  • \$\begingroup\$ Would you mind to clarify what kind of input and output this code expects? I'd like to try to write a SQL version (for the challenge) but it's a bit difficult without knowing those factors. Thanks! \$\endgroup\$ – Phrancis Jun 26 '15 at 15:45
  • \$\begingroup\$ Updated to include input, looking forward to your implementation! \$\endgroup\$ – AndyRoid Jun 26 '15 at 20:31
  • \$\begingroup\$ Awesome, I'll get to working on that and let you know what I come up with :) \$\endgroup\$ – Phrancis Jun 27 '15 at 0:25
  • \$\begingroup\$ Here is what I came up with: codereview.stackexchange.com/q/94860/42632 \$\endgroup\$ – Phrancis Jun 27 '15 at 3:15
15
\$\begingroup\$

I'd just say, WTF is this doing?

You're testing minutes and hours first as if they were the most important things. So with 10 minutes, 1 hours and 1000 days, you output something like "1 hours ago".

Maybe it's impossible as the variables all get computed from a single time interval. But you didn't show us.

I guess it's better for my sanity not to try too hard to figure out what it exactly does. So let me assume that all variable come from a single time interval. Then a condition like

minutes >= 60 && hours < 24

can be expressed like

1 <= hours && hours < 24

This doesn't buy us much as the order is still confusing:

  • " hours ago"
  • " days ago"
  • " minutes from now"
  • " months ago"

Again, not trying to find a system there.

What about something like

String suffix = minutes < 0 ? "ago" : "from now";

if (Math.abs(days) >= 365) {
    return (Math.abs(days) / 365) + " years " + suffix;
} else if (Math.abs(days) >= 2*30) {
    return (Math.abs(days) / 12) + " months " + suffix;
} else if (Math.abs(days) >= 30) {
    return days < 0 ? "Last month" : "Next month";
} else if (Math.abs(days) >= 1) {
    return Math.abs(days) + " days " + suffix;
} else if (Math.abs(hours) >= 1) {
    return Math.abs(hours) + " hours " + suffix;
} else {
    return Math.abs(minutes) + " minutes " + suffix;
}

I'm not claiming it's correct, but it's short and damn simple, so you find and fix all bugs in a few seconds.


So what I dislike is

  • The high count of cases, which could be cut in half using the abs idea stolen from Hosch250.
  • Placing two tests in each if when one is enough. You need no range tests if you do it systematically.
  • Even with range tests, there should be a clean order.

I always prefer to keep things simple from the very beginning. Otherwise, it can easily happen that you get a cool idea, but can't apply it because of the need to preserve compatibility with some quirks in the original.


Another disadvantage of starting with complicated stuff is that you may miss some simplifications the way I did above. Concerning correctness, all the divisions and comparisons with 356 and 30 are slightly wrong.

Note also that the task is not exactly defined. Given the dates 2000-12-31T23:23:00 and 2001-01-01T00:11:22, multiple answers are correct:

  • "48 minutes from now"
  • "nearly one hour from now"
  • "tomorrow"
  • "next month"
  • "next year"

And maybe also "next century" and "next millennium", but both are disputable (and irrelevant most of the time).

There's no clear solution for this and you should specify the behavior precisely and cover it by tests. The most straightforward solution would be to look at the calendar year first, which would give us the answer "next year". Pretty impractical, but every other solution needs some arbitrary choices.

I'd probably start with

String timeDifferenceString(long currentTimeMillis, long otherMillis) {
    final boolean isPast = otherMillis < currentTimeMillis;

    final Calendar first = Calendar.getInstance();
    first.setTimeInMillis(Math.min(currentTimeMillis, otherMillis));

    final Calendar second = Calendar.getInstance();
    second.setTimeInMillis(Math.max(currentTimeMillis, otherMillis));

    return timeDifferenceString(first, second, isPast);
}

so that no Math.abs is needed anymore. Obviously, Joda-Time or the corresponding JDK8 classes are a better choice than Calendar, but let's keep it simple for this answer.

Now you can write

private String timeDifferenceString(Calendar first, Calendar second, boolean isPast) {
    final String agoOrFromNow = isPast ? "ago" : "from now";
    final String lastOrNext = isPast ? "last " : "next ";

    final int years = second.get(Calendar.YEAR) - first.get(Calendar.YEAR);
    if (years > 1) {
        return years + " years " + agoOrFromNow;
    } else if (years > 0) {
        return lastOrNext + "year";
    }

    final int months = second.get(Calendar.MONTH) - first.get(Calendar.MONTH);
    if (months > 1) {
        return months + " months " + agoOrFromNow;
    } else if (years > 0) {
        return lastOrNext + "month";
    }

    ...

    return "now";
}

This leads to a rather lengthy but trivial method. Now you may want to split it as suggested by h.j.k. This is not easy, as the parts above only conditionally return something. Using a guard condition, this can be solved like this

    if (years > 0) {
        if (years > 1) {
            return years + " years " + agoOrFromNow;
        } else if (years > 0) {
            return lastOrNext + "year";
        } else {
            return "this year";
        }
    }

After extracting the method, you'll see that it's about the same for every time unit, so you may want to generalize it to

private String timeDifferenceString(int count, String unitName, boolean isPast) {
    if (count > 1) {
        return count + " " + unitName +  "s " + agoOrFromNow(isPast);
    } else if (count > 0) {
        return lastOrNext(isPast) + " " + unitName;
    } else {
        return "this " + unitName;
    }
}

Note that this won't work for other languages for many reasons including pluralization rules.

Now, you have a few trivial methods and one important one looking like

private String timeDifferenceString(Calendar first, Calendar second, boolean isPast) {
    final int years = second.get(Calendar.YEAR) - first.get(Calendar.YEAR);
    if (years > 0) {
        return timeDifferenceString(years, "year", isPast);
    }

    final int months = second.get(Calendar.MONTH) - first.get(Calendar.MONTH);
    if (months > 0) {
        return timeDifferenceString(months, "month", isPast);
    }

    ...

    return "now";
}

This is the right time for making changes, allowing you to output "48 minutes from now" instead of "next year" in my above example.

\$\endgroup\$
  • \$\begingroup\$ The API returns a unix stamp date, minutes hours and days is extracted out of it. Inside the method a new Date object called now is created and compared to that unix stamp date. \$\endgroup\$ – AndyRoid Jun 26 '15 at 1:51
  • \$\begingroup\$ I actually just realized something maaartinus, I could just use a compareTo for two date time objects, thoughts? \$\endgroup\$ – AndyRoid Jun 26 '15 at 2:03
  • \$\begingroup\$ @AndyRoid No thoughts. You could use it only for finding out what's bigger, but a sign of minutes can do it, too. Btw., do you know Yoda-Time? It has time intervals and would help to avoid things like days/356, which are imprecise. \$\endgroup\$ – maaartinus Jun 26 '15 at 2:05
  • \$\begingroup\$ Do you mean Joda-Time? Was just looking into Joda-Time actually, thanks for pointing that out! \$\endgroup\$ – AndyRoid Jun 26 '15 at 2:07
  • \$\begingroup\$ @AndyRoid Right, Joda, not Yoda. ;) Btw., it'd make sense to get the sign and ensure that all values are non-negative and save using abs many times. \$\endgroup\$ – maaartinus Jun 26 '15 at 2:16
10
\$\begingroup\$
else if (hours >= 24 && days <= 30) {
    return days + " days ago";
} else if (hours <= -24 && days >= -30) {
    return -days + " days from now";
}

You can combine these into one statement with the abs() operator:

if (Math.abs(hours) >= 24 && Math.abs(days) <= 30) {
    return Math.abs(days) + (hours > 0 ? " days ago" : " days from now");
}
\$\endgroup\$
  • \$\begingroup\$ this is really nice! What about readability any suggestions on that? \$\endgroup\$ – AndyRoid Jun 26 '15 at 1:17
  • 1
    \$\begingroup\$ The readability will dramatically improve if you reduce your code by half. It is pretty readable to me as it is, except for the excessive ifs. \$\endgroup\$ – Hosch250 Jun 26 '15 at 1:18
  • \$\begingroup\$ I just don't like a big block of if-else or else statements, but maybe its not that big of an issue \$\endgroup\$ – AndyRoid Jun 26 '15 at 1:23
  • \$\begingroup\$ I should point out that the negative implies a future event, so I can't accept this answer \$\endgroup\$ – AndyRoid Jun 26 '15 at 1:28
  • \$\begingroup\$ @AndyRoid I adjusted the output so it should return the correct value. \$\endgroup\$ – Hosch250 Jun 26 '15 at 1:33
10
\$\begingroup\$

Look for patterns and breaking down into methods

From the looks of it, you are trying to 'prettify' relative time representation given minutes, hours and days, such that negative values indicate the future and positive the past.

private static String getString(int minutes, int hours, int days) {
    // calculations here...
}

I suppose you will want to work from the larger time units first, and then iterate towards the smaller units. For example, if the time span is large - 1000 days - you will want to represent that in terms of years, rather than hours or minutes. If the time span is much smaller - 2 minutes - then the use of well-defined methods should allow us to conveniently skip the year/month/day/hour representations.

Therefore, the method now becomes something similar to:

private static String getString(int minutes, int hours, int days) {
    String years = toYears(days);
    if (years != null) {
        return years;
    }
    String months = toMonths(days);
    if (months != null) {
        return months;
    }
    String days = toDays(hours, days);
    if (days != null) {
        return days;
    }
    String hours = toHours(minutes, hours);
    return hours != null ? hours : toMinutes(minutes);
}

Of note, each of the methods, toYears(), toMonths(), toDays(), toHours() and toMinutes() only takes in the required time values to test if there is a suitable 'conversion' for it.

Simplification and cutting repetition

Using @Hosch250's excellent answer to simplify the numerical comparisons as a starting point, we have an implementation for toDays() already:

private static String toDays(int hours, int days) {
    if (Math.abs(hours) >= 24 && Math.abs(days) <= 30) {
        return Math.abs(days) + (hours > 0 ? " days ago" : " days from now");
    }
    return null;
}

However, as we go through the remaining methods, this pattern begins to get repetitive:

Math.abs(largerTimeUnit) + (smallerTimeUnit > 0 ? 
    " <larger_time_unit> ago " : " <larger_time_unit> from now")

As such, we can have another method to perform this step for us, across all the methods:

private static String toReference(int prefix, int suffixValue, String suffix) {
    return Math.abs(prefix) + suffix + (suffixValue > 0 ? "ago" : "from now");
}

The final result for all the methods:

private static String toMinutes(int minutes) {
    if (minutes >= -60 && minutes < 0) {
        return -minutes + " minutes from now";
    }
    return minutes + " minutes ago";
}

private static String toHours(int minutes, int hours) {
    if (Math.abs(minutes) >= 60 && Math.abs(hours) < 24) {
        return toReference(hours, minutes, " hours ");
    }
    return null;
}

private static String toDays(int hours, int days) {
    if (Math.abs(hours) >= 24 && Math.abs(days) <= 30) {
        return toReference(days, hours, " days ");
    }
    return null;
}

private static String toMonths(int days) {
    int result = days / 30;
    if (Math.abs(days) > 30) {
        return Math.abs(result) == 1 ? (result == 1 ? "Last" : "Next") + " month" : 
                toReference(result, days, " months ");
    }
    return null;
}

private static String toYears(int days) {
    int result = days / 365;
    if (Math.abs(days) >= 365) {
        return Math.abs(result) == 1 ? (result == 1 ? "Last" : "Next") + " year" : 
                toReference(result, days, " years ");
    }
    return null;
}

toMinutes() is the exception here as it is simply the last case for us: when all conditions fail, we simply return minutes + " minutes ago".

Conclusion

By identifying patterns in your possible String representations, you can further split your conditions into better-defined methods, which then allows for code reuse.

It's also recommended to create unit testing for these methods. :)


Final tip: If you happen to be able to implement this in Java 8 in the future, getString() can be simplified using the Supplier class and a healthy dose of method references:

private static String getString(int minutes, int hours, int days) {
    return pick(() -> toYears(days), () -> toMonths(days), () -> toDays(hours, days), 
                () -> toHours(minutes, hours), () -> toMinutes(minutes));
}

@SafeVarargs
public static <T> T pick(Supplier<T>... suppliers) {
    for (Supplier<T> current : suppliers) {
        T result = current.get();
        if (result != null) {
            return result;
        }
    }
    throw new IllegalArgumentException("No non-null results generated.");
}

What this does is to test for each method's conversion in the form of a Supplier, and return-ing the first non-null result.

edit Modified final-tip's code as the Optional-based suggestion was... completely wrong.

\$\endgroup\$
  • \$\begingroup\$ very very thorough \$\endgroup\$ – AndyRoid Jun 26 '15 at 3:05
  • \$\begingroup\$ and bonus points for the unit testing suggestion \$\endgroup\$ – AndyRoid Jun 26 '15 at 3:05
  • \$\begingroup\$ Could you optimize it for Java 1.7, most people are not on 1.8 yet specifically for Android anyways it causes issues. I really like this, some critiques include reusing abs(...) a lot though. This might be the best answer out of all, but it is still not as complete as maaartinus's \$\endgroup\$ – AndyRoid Jun 26 '15 at 3:07
  • 2
    \$\begingroup\$ The only part concerning 1.8 is in the final tip, everything in the main chunk will work < Java 8. :) \$\endgroup\$ – h.j.k. Jun 26 '15 at 3:24
5
\$\begingroup\$

There's little sense in using an elif if you're returning anyway.

if (minutes >= 60 && hours < 24) {
    return hours + " hours ago";
}

if (hours >= 24 && days <= 30) {
    return days + " days ago";
}

if (minutes <= -60 && hours > -24) {
    return -hours + " hours from now";
}

if (hours <= -24 && days >= -30) {

It adds to the line count, but adds something to the readability.

\$\endgroup\$
  • \$\begingroup\$ good catch on pointing this out. \$\endgroup\$ – AndyRoid Jun 26 '15 at 1:31
  • 2
    \$\begingroup\$ @AndyRoid Some (including me) consider using "else-if" clearer, despite the "return". The real problems are IMHO 1. the high count of cases, 2. placing two tests in each if when one is enough, 3. no system in the cases. \$\endgroup\$ – maaartinus Jun 26 '15 at 1:49
  • \$\begingroup\$ @maaartinus I'm often in that camp myself, but I recommend it here because of the "tight" bracing style. In C# I don't think it would have been a worthwhile change. \$\endgroup\$ – RubberDuck Jun 26 '15 at 2:56
5
\$\begingroup\$

First of all, let's keep in mind that calculating date differences accurately is really tricky stuff. For example, if you want to support "x days ago" and "last month", then it's not trivial to choose the right one, given a time difference of 30 days, as it depends on the length of the current month and the day. And this is only scratching the surface, because strictly speaking, due to daylight savings and other fun stuff, even "yesterday" or "23 hours ago" might be questionable. So I will assume that accuracy is not critical. Joda time is a popular library for accurate date time calculations.

I propose an alternative implementation with fewer conditionals, more straightforward logic, flexible and extensible with regards to time units of interest:

  • design a time delta class
    • contains an ordered set of time elements
    • a time element is composed of a time unit (year, month, day, hour, ...), and the count, for example "3 days"
    • contains a sign to indicate past, future, now
    • the constructor takes 2 dates as parameters, and computea the time elements and the sign from the difference. This process is analogous to building the digits of an n-base number when converting from decimal using successive modulo and division operations going from small digits to large
  • create an instance of time delta from the 2 dates that you have
  • the sign of the time delta decides between past, future, now
    • if the sign is zero ("now ", identical dates), you can stop processing (your current program produces "0 minutes ago" for this case)
  • get the highest time element from the time delta
  • from the non-zero sign and the time element, you can build the output with only a few conditionals to decide between one of the templates you need:
    • "(x) (time unit)s (suffix = ago | from now)"
    • "(prefix = next | last) (time unit)"
\$\endgroup\$
3
\$\begingroup\$

Not the best tool for the job.

I frequently use database systems strictly for date-time calculations. Because they are really good at it. A Code Review regular once said:

Never calculate date time by hand. This rule is more important than not using GoTo statements...

Database connectors for Java are easy to make, and you just need a few stored functions/procedures to do the job more accurately than any human could.

Example?

SELECT DATEADD(MINUTE, 29723947, CURRENT_TIMESTAMP);
-------------
2071-12-30 17:05:59

Another?

DECLARE @FallOfBerlinWall DATE = '1990-10-03';
SELECT DATEDIFF(SECOND, @FallOfBerlinWall, CURRENT_TIMESTAMP);
--------- 
780372479

TL:DR;

Don't calculate date-time by hand.

\$\endgroup\$
  • \$\begingroup\$ This is a great idea. I am doing this on a mobile product, so maybe using the SQLite DB on the device? \$\endgroup\$ – AndyRoid Jun 26 '15 at 2:14
  • \$\begingroup\$ SQLite sounds like a good idea, indeed \$\endgroup\$ – Phrancis Jun 26 '15 at 2:15
  • 2
    \$\begingroup\$ I find the idea perverted, because of 1. the need to have some DB around, 2. inefficiency (round trip to DB), 3. exposing yourself to various DB quirks. +++ That said, it's an original idea. \$\endgroup\$ – maaartinus Jun 26 '15 at 2:20
  • \$\begingroup\$ How would you solve this particular problem in SQL? \$\endgroup\$ – 200_success Jun 26 '15 at 14:59
  • \$\begingroup\$ @200_success Hard to say exactly, since the OP didn't explain what the input is supposed to be and the questions is mostly just a block of code. Likely, I'd create a stored procedure that accepts whatever arguments the above monstrosity of manual date calculations is processing, and have it return whatever it is supposed to return. \$\endgroup\$ – Phrancis Jun 26 '15 at 15:42

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.