9
\$\begingroup\$

The following question was taken from Absolute Java 5th ed. by Walter Savitch:

Write a program that outputs the number of hours, minutes, and seconds that corresponds to 50,391 total seconds. The output should be 13 hours, 59 minutes, and 51 seconds. Test your program with a different number of total seconds to ensure that it works for other cases.

This is the code that I have written:

    public class Question7 {

    private static final int MINUTES_IN_AN_HOUR = 60;
    private static final int SECONDS_IN_A_MINUTE = 60;

    public static void main(String[] args) {

        int seconds = 50391;

        System.out.println(timeConversion(seconds));

    }

    private static String timeConversion(int totalSeconds) {
        int hours = totalSeconds / MINUTES_IN_AN_HOUR / SECONDS_IN_A_MINUTE;
        int minutes = (totalSeconds - (hoursToSeconds(hours)))
                / SECONDS_IN_A_MINUTE;
        int seconds = totalSeconds
                - ((hoursToSeconds(hours)) + (minutesToSeconds(minutes)));

        return hours + " hours " + minutes + " minutes " + seconds + " seconds";
    }

    private static int hoursToSeconds(int hours) {
        return hours * MINUTES_IN_AN_HOUR * SECONDS_IN_A_MINUTE;
    }

    private static int minutesToSeconds(int minutes) {
        return minutes * SECONDS_IN_A_MINUTE;
    }
}
\$\endgroup\$

2 Answers 2

17
\$\begingroup\$

Your usage of auxiliary functions is not consistent: Some computations are done inline, e.g.

int hours = totalSeconds / MINUTES_IN_AN_HOUR / SECONDS_IN_A_MINUTE;

and other (equally simple) computations are done using the functions hoursToSeconds() and minutesToSeconds().

But my main point is that the calculation is too complicated, and extending it to more time units (e.g. days) would make it even more complicated.

It is easier if you compute the lowest time unit first (seconds) using the remainder operator %, and then proceed to the higher time units (minutes, then hours).

Then you don't need additional functions for the conversion and therefore you can restrict the scope of the conversion constants to the timeConversion() function.

private static String timeConversion(int totalSeconds) {

    final int MINUTES_IN_AN_HOUR = 60;
    final int SECONDS_IN_A_MINUTE = 60;

    int seconds = totalSeconds % SECONDS_IN_A_MINUTE;
    int totalMinutes = totalSeconds / SECONDS_IN_A_MINUTE;
    int minutes = totalMinutes % MINUTES_IN_AN_HOUR;
    int hours = totalMinutes / MINUTES_IN_AN_HOUR;

    return hours + " hours " + minutes + " minutes " + seconds + " seconds";
}
\$\endgroup\$
0
4
\$\begingroup\$

I know that efficiency is the last thing to worry about in such a function. However I would rewrite answer of @Martin R as:

private static String timeConversion(int seconds) {

    final int MINUTES_IN_AN_HOUR = 60;
    final int SECONDS_IN_A_MINUTE = 60;

    int minutes = seconds / SECONDS_IN_A_MINUTE;
    seconds -= minutes * SECONDS_IN_A_MINUTE;

    int hours = minutes / MINUTES_IN_AN_HOUR;
    minutes -= hours * MINUTES_IN_AN_HOUR;

    return hours + " hours " + minutes + " minutes " + seconds + " seconds";
}

I always feel not comfortable when both operations a/b and a%b are computed separately, since the second one can be computed in terms of the first by means of sum and multiplication (which are much more efficient operations). Also this alternative "stream of thinking", i.e. modify the input values until they satisfy the requirements, requires less use of imagination to find the names of variables.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.