5
\$\begingroup\$

Problem:

Write a program that, given an age in seconds, calculates how old someone is in terms of a given planet's solar years.

  • Earth: orbital period 365.25 Earth days, or 31557600 seconds
  • Mercury: orbital period 0.2408467 Earth years
  • Venus: orbital period 0.61519726 Earth years
  • Mars: orbital period 1.8808158 Earth years
  • Jupiter: orbital period 11.862615 Earth years
  • Saturn: orbital period 29.447498 Earth years
  • Uranus: orbital period 84.016846 Earth years
  • Neptune: orbital period 164.79132 Earth years

So if you were told someone were 1,000,000,000 seconds old, you should be able to say that they're 31 Earth-years old.

Code:

import java.math.BigDecimal;
import java.math.RoundingMode;

public class SpaceAge {
  private static final double EARTH_YEARS_IN_SECONDS = 365.25 * 24 * 60 * 60;
  private double ageInSeconds;

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

  public double getSeconds() {
    return ageInSeconds;
  }

  /** Orbital period relative to Earth. */
  private enum RelativeOrbitalPeriod {
    MERCURY(0.2408467),
    VENUS  (0.61519726),
    MARS   (1.8808158),
    JUPITER(11.862615),
    SATURN (29.447498),
    URANUS (84.016846),
    NEPTUNE(164.79132);

    private final double ratio;

    RelativeOrbitalPeriod(double ratio) {
      this.ratio = ratio;
    }

    public double get() {
      return ratio;
    }
  }

  public double onEarth() {
    //return BigDecimal.valueOf(ageInSeconds)
      //.divide(BigDecimal.valueOf(60), 2, RoundingMode.FLOOR)  // age in minutes
      //.divide(BigDecimal.valueOf(60), 2, RoundingMode.FLOOR)  // age in hours
      //.divide(BigDecimal.valueOf(24), 2, RoundingMode.FLOOR)  // age in days
      //.divide(BigDecimal.valueOf(365.25), 2, RoundingMode.FLOOR) // age in years
      //.doubleValue();
    return ageInSeconds / EARTH_YEARS_IN_SECONDS;
  }

  public double onMercury() {
    return onEarth() / RelativeOrbitalPeriod.MERCURY.get();
  }

  public double onVenus() {
    return onEarth() / RelativeOrbitalPeriod.VENUS.get();
  }

  public double onMars() {
    return onEarth() / RelativeOrbitalPeriod.MARS.get();
  }

  public double onJupiter() {
    return onEarth() / RelativeOrbitalPeriod.JUPITER.get();
  }

  public double onSaturn() {
    return onEarth() / RelativeOrbitalPeriod.SATURN.get();
  }

  public double onUranus() {
    return onEarth() / RelativeOrbitalPeriod.URANUS.get();
  }

  public double onNeptune() {
    return onEarth() / RelativeOrbitalPeriod.NEPTUNE.get();
  }
}

Test Suite:

import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class SpaceAgeTest {

    private static final double MAXIMUM_DELTA = 1E-02;

    @Test
    public void ageInSeconds() {
        SpaceAge age = new SpaceAge(1000000);

        assertEquals(1000000, age.getSeconds(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnEarth() {
        SpaceAge age = new SpaceAge(1000000000);

        assertEquals(31.69, age.onEarth(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnMercury() {
        SpaceAge age = new SpaceAge(2134835688);

        assertEquals(67.65, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(280.88, age.onMercury(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnVenus() {
        SpaceAge age = new SpaceAge(189839836);

        assertEquals(6.02, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(9.78, age.onVenus(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnMars() {
        SpaceAge age = new SpaceAge(2329871239L);

        assertEquals(73.83, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(39.25, age.onMars(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnJupiter() {
        SpaceAge age = new SpaceAge(901876382);

        assertEquals(28.58, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(2.41, age.onJupiter(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnSaturn() {
        SpaceAge age = new SpaceAge(3000000000L);

        assertEquals(95.06, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(3.23, age.onSaturn(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnUranus() {
        SpaceAge age = new SpaceAge(3210123456L);

        assertEquals(101.72, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(1.21, age.onUranus(), MAXIMUM_DELTA);
    }

    @Test
    public void ageOnNeptune() {
        SpaceAge age = new SpaceAge(8210123456L);

        assertEquals(260.16, age.onEarth(), MAXIMUM_DELTA);
        assertEquals(1.58, age.onNeptune(), MAXIMUM_DELTA);
    }
}

Design Decisions:

  • First of all I would like to comment that I spent so much time on studying about floating point and BigDecimal and on implementation I found all my test are failing with few margins :(, later I found that problem curator didn't think like that way and rather the solution was too simple.
  • Now, instead of using magic constants I have used enums to enhance readability.
  • Used descriptive names whenever possible.

Questions:

  • I find that the class is loaded with too much responsibilities, it knows too much(eg: how to get the age in Mars etc.), is this fine?

  • Now, suppose I need add another planet then I would need to add a method and modify the enum which feels like to violate Open Closed Principle, am I overthinking?

Reference: http://exercism.io/exercises/java/space-age/readme

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

Using an enumeration is a great idea: it centralizes the ratio for all planets. But its name is incorrect: you named it RelativeOrbitalPeriod, implying that this enumeration only purpose is to store the relative orbital period to Earth. However, this is focusing too much on how the class is doing what it does, instead of what it represents.

Conceptually, you want to model your classes from a high-level perspective: if you forget about the implementation, a name RelativeOrbitalPeriod sounds like this class would store periods; but that's not what it stores: it stores planets. And each planet has a corresponding period relative to Earth. That is to say, the relative period is a property of each planets.

A good name for the enum could simply be Planet. If tomorrow, you were to add to your model some other property of a planet (like the distance between that planet and the Sun), you would just need to add an attribute to the class Planet (like distanceFromSun). With your current model, that would mean having a field RelativeOrbitalPeriod.distanceFromSun, which is awkward; a field Planet.distanceFromSun would be a lot clearer.


This also has a second consequence, that shows a dissymmetry in your current implementation. All onXxx() methods look like:

public double onMercury() {
  return onEarth() / RelativeOrbitalPeriod.MERCURY.get();
}

but onEarth() looks like:

public double onEarth() {
  return ageInSeconds / EARTH_YEARS_IN_SECONDS;
}

This is bizarre: the Earth is also planet, just like Venus or Mercury; why should there be a different code path for it?

This is because the enumeration is missing one planet, namely EARTH, which would have a ratio of 1. So this value needs to be added to our enum, so that we have:

private enum Planet {

    EARTH(1),
    MERCURY(0.2408467),
    VENUS(0.61519726),
    MARS(1.8808158),
    JUPITER(11.862615),
    SATURN(29.447498),
    URANUS(84.016846),
    NEPTUNE(164.79132);

    private final double ratio;

    Planet(double ratio) {
        this.ratio = ratio;
    }

    public double get() {
        return ratio;
    }

}

A third comment is about the responsibility of each class. In OOP, an object should keep its internal representation to itself and try as much as possible to not let it leak. In your current design, you are leaking the fact that each duration on each planet is calculated with a relative period to Earth. This is because you have added a get() method inside the enumeration that returns the ratio to apply.

A solution that would hide this fact to the client, and keep that ratio internal, would simply be to let each planet calculate the right age:

public double forSeconds(double seconds) {
    return seconds / (EARTH_YEARS_IN_SECONDS * ratio);
}

With this change, ratio is kept internal. The advantage is that, if tomorrow you were to change that logic (let's say tomorrow the age on a planet depends on its distance from the Sun), you only need to change the Planet class: the caller is unaware of it and that's good. Put another way, the client wants to know the age on a planet given a number of seconds, and doesn't need to know how to calculate it; this responsibility is up to each planet.


Finally, the current public API exposes each onXxx for each planet. This is wanted by the tests, why not. It serves as a short-cut for the client. It is true that if you were to add another planet, you would need to add another method to keep the consistency. A slight refactor is still possible; with the above changes, we would now have, for example,

public double onSaturn() {
    return Planet.SATURN.forSeconds(ageInSeconds);
}

public double onUranus() {
    return Planet.URANUS.forSeconds(ageInSeconds);
}

This is possible to refactor it to:

public double onUranus() {
    return on(Planet.URANUS);
}

public double onNeptune() {
    return on(Planet.NEPTUNE);
}

private double on(Planet planet) {
    return planet.forSeconds(ageInSeconds);
}

This would eliminate the duplicate calls to forSeconds.


By implementing all of the comments above, you could have:

public class SpaceAge {

    private static final double EARTH_YEARS_IN_SECONDS = 365.25 * 24 * 60 * 60;

    private double ageInSeconds;

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

    public double getSeconds() {
        return ageInSeconds;
    }

    /** Orbital period relative to Earth. */
    private enum Planet {
        EARTH(1),
        MERCURY(0.2408467),
        VENUS(0.61519726),
        MARS(1.8808158),
        JUPITER(11.862615),
        SATURN(29.447498),
        URANUS(84.016846),
        NEPTUNE(164.79132);

        private final double ratio;

        Planet(double ratio) {
            this.ratio = ratio;
        }

        public double forSeconds(double seconds) {
            return seconds / (EARTH_YEARS_IN_SECONDS * ratio);
        }
    }

    public double onEarth() {
        return Planet.EARTH.forSeconds(ageInSeconds);
    }

    public double onMercury() {
        return Planet.MERCURY.forSeconds(ageInSeconds);
    }

    public double onVenus() {
        return Planet.VENUS.forSeconds(ageInSeconds);
    }

    public double onMars() {
        return Planet.MARS.forSeconds(ageInSeconds);
    }

    public double onJupiter() {
        return Planet.JUPITER.forSeconds(ageInSeconds);
    }

    public double onSaturn() {
        return Planet.SATURN.forSeconds(ageInSeconds);
    }

    public double onUranus() {
        return on(Planet.URANUS);
    }

    public double onNeptune() {
        return on(Planet.NEPTUNE);
    }

    private double on(Planet planet) {
        return planet.forSeconds(ageInSeconds);
    }

}

One final note about BigDecimal: as you found out, calculations on double can quickly carry a lot of errors. If precise calculation is needed across multiple operations, a BigDecimal should be used. Note that switching to BigDecimal also carries a performance cost: operations will be a lot slower compared to primitive doubles. In this case, I don't think it is really necessary: we have one multiplication, a division and we're only interested in 2 decimal precision; using a double is enough.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Here enum is used as a mapping now, how we know what the names of each planet are mapped to? the number isn't telling me anything. Apart from that we have given extra responsibility to it to do some calculation. Regarding OCP the problem is still there we are modifying the original class, you just made the code more readable but the problem is still there. \$\endgroup\$ – CodeYogi May 12 '16 at 2:36
  • \$\begingroup\$ @CodeYogi The number can be documented by adding Javadoc on the enum constructor. The OCP doesn't really apply here since we're using an enum in the first place, which means that we need to code all the values of the enum (thus not extensible). But I don't think this is a problem in this case: planets are fixed; conceptually, no planets will be added or removed, they will stay the same and this is why using an enum is a good idea (forgetting about Pluto :p). \$\endgroup\$ – Tunaki May 12 '16 at 7:31
  • \$\begingroup\$ Yes, its an irony here that planets are fixed but still I was more interested in design techniques. I think the problems are not of that kind or we are not able to get out of single class doing all athletics phase ;p \$\endgroup\$ – CodeYogi May 12 '16 at 9:43

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.