Skip to main content
added 3942 characters in body
Source Link
maaartinus
  • 13.5k
  • 1
  • 34
  • 73

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.


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.

added 475 characters in body
Source Link
maaartinus
  • 13.5k
  • 1
  • 34
  • 73

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. The abs idea was stolen


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 Hosch250the 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.

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. The abs idea was stolen from Hosch250.

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.

Source Link
maaartinus
  • 13.5k
  • 1
  • 34
  • 73

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. The abs idea was stolen from Hosch250.