4
\$\begingroup\$

I had an exercise to solve, written in German. I try to translate it:

Write a program that takes an amount of seconds. The program than has to output the number of years, days, hours and seconds.

My Solution

public class Main {
  public static void main(String[] args) {
    final int HOUR_IN_SECONDS = 60 * 60;
    final int DAY_IN_SECONDS = HOUR_IN_SECONDS * 24;
    final int YEAR_IN_SECONDS = DAY_IN_SECONDS * 365;

    int seconds = 1000000000;

    // Jahre berechnen
    int years = seconds / YEAR_IN_SECONDS;
    seconds = seconds - years * YEAR_IN_SECONDS;
    System.out.println("Jahre: " + years);

    // Tage berechnen
    int days = seconds / DAY_IN_SECONDS;
    seconds = seconds - days * DAY_IN_SECONDS;
    System.out.println("Tage: " + days);

    // Stunden berechnen
    int hours = seconds / HOUR_IN_SECONDS;
    seconds = seconds - hours * HOUR_IN_SECONDS;
    System.out.println("Stunden: " + hours);

    // Minuten berechnen
    int minutes = seconds / 60;
    seconds = seconds - minutes * 60;
    System.out.println("Minuten: " + minutes);

    // Sekunden ausgeben
    System.out.println("Sekunden: " + seconds);
  }
}

This code works, but is it also beautiful enough and easy to read?

\$\endgroup\$
  • 2
    \$\begingroup\$ Can you use any libraries? Because converting seconds to years and so on is not really that straight forward. You need to take leap years into account. Why not use Java Time? \$\endgroup\$ – Saeed Entezari Jan 15 at 15:02
  • 1
    \$\begingroup\$ Its a beginner question. I am not allowed to use libraries. The exercise wants me to assume, a year has always 365 days. I have given my solution to a beginner as an example solution so that he can learn. I just wanted to check if my solution doenst teaches him bad style. \$\endgroup\$ – Dexter Thorn Jan 15 at 16:30
  • \$\begingroup\$ If the value is simply "seconds" without a fixed position in time (as opposed to "seconds since Jan 1st 1970") the purpose is to make a conversion between time units, ignoring leap years and seconds. \$\endgroup\$ – TorbenPutkonen Jan 16 at 11:20
3
\$\begingroup\$

I have some suggestion for you.

1) The naming convention of the variables HOUR_IN_SECONDS, DAY_IN_SECONDS and YEAR_IN_SECONDS is not correct, since they are not constant; missing the static keyword. I suggest that you extract them in the class level.

public static final int HOUR_IN_SECONDS = 60 * 60;
public static final int DAY_IN_SECONDS = HOUR_IN_SECONDS * 24;
public static final int YEAR_IN_SECONDS = DAY_IN_SECONDS * 365;

public static void main(String[] args) {
   //[...]
}

2) I suggest that you create a method that take the number of seconds and print the result.

public static void main(String[] args) {
  int seconds = 1000000000;
  printFromSeconds(seconds);
}

private static void printFromSeconds(int seconds) {
    //[...]
}

For the rest, I think it's ok since the variable seconds is closely linked with the other components.

\$\endgroup\$
2
\$\begingroup\$

First off a typical bad habit many beginners get into is, putting everything in main. If the code is only doing one thing and can be self contained a function will do. Otherwise, use a class. In this case I would suggest a class with static fields and functions.

Try to avoid using magic numbers, strings, etc.. Use final variables so that they can be named. In many cases it can be difficult to figure out why a particular value was used, in a years time or longer.

There is a fair bit of repetition in your code. A helper function would clean that up.

Use formatted strings instead concatenation(+).

Since you seem to be using one language for programming and a different one for output, I would deduce that it's possible that other languages may also be required for the output. to this end a simple TimeNames class to store different string arrays with the time labels in different languages.

TimeNames.java

public class TimeNames{
    public enum Languages{
        English,
        German
    }
    static final String[] germanNames = {
        "Jahre",
        "Tage",
        "Stunden",
        "Minuten",
        "Sekunden"
    };
    static final String[] englishNames = {
        "Years",
        "Days",
        "Hours",
        "Minutes",
        "Seconds"
    };
    static String[] names = englishNames;    

    public static void changeLanguage(Languages language){
        switch(language){
            case English:
            names = englishNames;
            break;
            case German:
            names = germanNames;
            break;
            default:
            names = englishNames;
        }       
    }
    public static String getYears(){
        return names[0];
    }
    public static String getDays(){
        return names[1];
    }
    public static String getHours(){
        return names[2];
    }
    public static String getMinutes(){
        return names[3];
    }
    public static String getSeconds(){
        return names[4];
    } 
}

SecondsToTime.java

public class SecondsToTime{
    static final int SECS_IN_MINUTES = 60;
    static final int MINS_IN_HOUR = 60;
    static final int HOURS_IN_DAY = 24;
    static final int DAYS_IN_YEAR = 365;

    static final int MIN_CONV = SECS_IN_MINUTES;
    static final int HOUR_CONV = MIN_CONV * MINS_IN_HOUR;
    static final int DAY_CONV = HOUR_CONV * HOURS_IN_DAY;
    static final int YEAR_CONV = DAY_CONV * DAYS_IN_YEAR;

    static long years = 0;
    static long days = 0;
    static long hours = 0;
    static long minutes = 0;
    static long seconds = 0;

    public static String secondsToTime(long secs){
        long[] secsTotal = {secs};
        years = convert(YEAR_CONV,secsTotal);
        days = convert(DAY_CONV,secsTotal);
        hours = convert(HOUR_CONV,secsTotal);
        minutes = convert(MIN_CONV, secsTotal);
        seconds = secsTotal[0];
        return String.format("%s: %d\n%s: %d\n%s: %d\n%s: %d\n%s: %d", TimeNames.getYears(), years, TimeNames.getDays(), days,
                TimeNames.getHours(), hours, TimeNames.getMinutes(), minutes, TimeNames.getSeconds(), seconds);
    }

    private static long convert (final int factor, long[] secs){
        int temp = (int)secs[0]/factor;
        secs[0] -= temp * factor;
        return temp;
    }
}

Main.java

public class Main{
    public static void main(String[] args) {
        String time = SecondsToTime.secondsToTime(1000000000);
        System.out.println(time);
        TimeNames.changeLanguage(TimeNames.Languages.German);
        time = SecondsToTime.secondsToTime(1000000000);
        System.out.println(time);
    } 
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the code! I have to mention that the beginner I have to teach Java doesnt know how to program in an object oriented manner yet - he is about learning data types, control instructions (if, else, for while) and algorithmic thinking yet.But in some days he will get into object oriented programming, so thank you for that! \$\endgroup\$ – Dexter Thorn Jan 16 at 6:20
  • 1
    \$\begingroup\$ If you don't want the functions and variables in separate classes, you can put them inside the main class. You'll just have to change the way they are called. \$\endgroup\$ – tinstaafl Jan 16 at 6:29
  • \$\begingroup\$ You should include an intermediate data structure that provides the time split into different units, not just output the time as a string. Internally it would be a Map<TimeUnit, Long> with appropriate accessor methods. I mean... since we got localization and all. :) \$\endgroup\$ – TorbenPutkonen Jan 16 at 11:25
1
\$\begingroup\$

I found a little inconsistency in the following part of the code:

// Minuten berechnen
int minutes = seconds / 60;
seconds = seconds - minutes * 60;
System.out.println("Minuten: " + minutes);

In all the other paragraphs you use named constants. You should follow that pattern here as well. MINUTE_IN_SECONDS is a perfect name for the 60.

The following two statements are equivalent. The second is idiomatic Java, the first is unnecessarily verbose.

seconds = seconds - minutes * 60;
seconds -= minutes * 60;

In the following declaration, I prefer the second variant since it is closer to the natural language "1 day is 24 hours" when being read aloud.

int DAY_IN_SECONDS = HOUR_IN_SECONDS * 24;
int DAY_IN_SECONDS = 24 * HOUR_IN_SECONDS;

A completely different way of splitting the total seconds x into the different time units is:

int seconds = x % 60;
x /= 60;

int minutes = x % 60;
x /= 60;

int hours = x % 24;
x /= 24;

int days = x % 365;
x /= 365;

int years = x;

This way you don't need the named constants since there are 60 seconds, 24 hours and 365 days, and all the numbers that are spelled out are written close to the name of their corresponding time unit.

Sure, the code looks like it has a lot of duplication, but there's no easy way around that since Java doesn't support multiple return values per method.

\$\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.