0
\$\begingroup\$

I was wondering if it is better to use:

    double onJupiter() {
     return AgeInSeconds / (jupiterOrbitalInEarthYears * ageInEarthSeconds);
    }

    double onMars() {
     return inputAgeInSeconds / (marsOrbitalInEarthYears * ageInEarthSeconds); 
 }

Or:

  double onJupiter() {
        return onEarth() / JUPITER_ORBIT_RATIO;
    }

  double onMars() {
    return onEarth() / MARS_ORBIT_RATIO;
  }

In the second option, notice I called the onEarth() method twice. Does calling onEarth() create a brand new Stack (a bit like in recursion), hence it is not good to use? Because if I have a bunch of methods like onMars(), onMercury etc and they all call onEarth(), then would I end up creating a lot of new stacks?

Here are the Java classes in question:

Class 1:

class SpaceAge {

    private final double AgeInSeconds;
    private static final double ageInEarthSeconds = 31557600;
    private static final double marsOrbitalInEarthYears = 0.2408467;
    private static final double jupiterOrbitalInEarthYears = 11.862615;

    SpaceAge(double seconds) {
        this.AgeInSeconds = seconds;
    }

    double getSeconds() {
        return AgeInSeconds;
    }

    double onEarth() {
        return AgeInSeconds / ageInEarthSeconds;
    }

    double onJupiter() {
        return AgeInSeconds / (jupiterOrbitalInEarthYears * ageInEarthSeconds);
    }


    double onMars() {
        return AgeInSeconds / (marsOrbital * ageInEarthSeconds);
    }

}

Class 2:

class SpaceAge {

    private static final double EARTH_YR_IN_SECS = 31557600;
    private static final double MARS_ORBIT_RATIO = 1.8808158;
    private static final double JUPITER_ORBIT_RATIO = 11.862615;


    private final double ageInSeconds;

    SpaceAge(final double seconds) {
        this.ageInSeconds = seconds;
    }

    double getSeconds() {
        return this.ageInSeconds;
    }

    double onEarth() {
        return this.ageInSeconds / EARTH_YR_IN_SECS;
    }

    double onJupiter() {
        return onEarth() / JUPITER_ORBIT_RATIO;
    }

    double onMars() {
        return onEarth() / MARS_ORBIT_RATIO;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Conventionally in Java, the names of fields start with lower case letters. Sticking to that convention will make it easier to read the code. As you can see, even the syntax highlighter treats AgeInSeconds differently than all other fields. Maybe this is simply a mistake, because in the second class, you named the field ageInSeconds. \$\endgroup\$ – Stingy Aug 3 '17 at 0:44
1
\$\begingroup\$

Don't care about that. Unless you can prove that it creates a bottleneck, just stick with whatever seems the most readable and maintainable option. prefer the latter.

I'd also rename the methods. onEarth is too generic. What exactly on onEarth? Weeks? Months? Years? If it's years, call it getYearsOnEarth or something like this.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. Yes, I guess I cannot prove that, but what if I have millions of methods and each of them calls the onEarth() method? I wonder if it would make a difference? \$\endgroup\$ – M Garp Aug 1 '17 at 20:06
  • \$\begingroup\$ @MGarp It's very unlikely. A compiler can optimize it. \$\endgroup\$ – kraskevich Aug 1 '17 at 20:21
  • \$\begingroup\$ @MGarp By "prove" I mean running the code, seeing that at it's too slow and then finding the bottleneck with a profiler. \$\endgroup\$ – kraskevich Aug 1 '17 at 20:23
0
\$\begingroup\$

Some additional suggestions:

  • ageInEarthSeconds can be an int instead of a double.
  • ageInEarthSeconds seems to represent the number of seconds in an average earth year. The name of the field doesn't really tell that. Maybe a more descriptive name would be secondsPerEarthYear or something similar.
  • The average number of days in a Gregorian calendar year is actually 365.2425, not 365.25, which is your ageInEarthSeconds converted to days.
  • You could make your program more flexible/extensible by defining an enum Planet that contains a final field orbitalPeriodInSeconds (might need to be long, depending on the possible values). That way, you don't have to define a separate method onEarth(), onJupiter() etc. for every planet, but only one single method:

    double ageInOrbitalPeriods(Planet planet) {
        return ageInSeconds / planet.orbitalPeriodInSeconds;
    }
    

    This will, incidentally, also take care of the problem you originally asked about in your question.

| improve this answer | |
\$\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.