5
\$\begingroup\$

Our solar system has 8 planets, including earth. Our calendar has some very confusing concepts such as leap years, which I still don't fully understand. In fact, since where I live, we use a different solar calendar that is different from the Gregorian reforms of the Julian Western calendar, I think I'm born in March but I should be born in February! Anyways, I attempted to create a simple program that calculates any given age, subtracting the leap days of each four years, in any planet of our solar system except Pluto which I believe is not a planet. I'm not sure about it.

def calculate_days(age):
    leap_days = 0 #holds number of leap days
    leap_years_list = [i for i in range(age) if i % 4 == 0] #holds the essence of leap years

    for j in leap_years_list:
        if j % 4 == 0 or j % 400 == 0 and j % 100 != 0:
            leap_days += 1 #if leap years are divisible by four and not divisible by a hundred, add to leap days

    return (age * 365) - leap_days


def calculate_celestial_age(planet, age):
    number_of_days = calculate_days(age) #calculate the days in age

    assert str.islower(planet), "Planet name must be entered in lower case e.g. 'mercury'" #if planet name is in lowercase, give error

    days_in_year = {"mercury": 88, "venus": 224, "earth": 365, "mars": 687, "jupiter": 4332, "saturn": 10759, "uranus": 30688,
                    "neptune": 60182} #days in each planet's year

    if planet == "mercury":
        return number_of_days / days_in_year["mercury"]
    elif planet == "venus":
        return number_of_days / days_in_year["venus"]
    elif planet == "earth":
        return number_of_days / days_in_year["earth"]
    elif planet == "mars":
        return number_of_days / days_in_year["mars"]
    elif planet == "jupiter":
        return number_of_days / days_in_year["jupiter"]
    elif planet == "saturn":
        return number_of_days / days_in_year["saturn"]
    elif planet == "uranus":
        return number_of_days / days_in_year["uranus"]
    elif planet == "neptune":
        return number_of_days / days_in_year["neptune"]
    else:
        raise Exception("Unknown Planet! Are you sure you've enter a planet?")


age = 23

planets = ["mercury", "venus", "earth", "mars", "jupiter", "saturn", "uranus", "neptune"]

for planet in planets:
    print("{0}: {1}".format(planet, calculate_celestial_age(planet, age)))

For my age, 23, I got these results:

mercury: 95.3409090909091

venus: 37.455357142857146

earth: 22.986301369863014

mars: 12.212518195050945

jupiter: 1.9367497691597415

saturn: 0.7798122502091273

uranus: 0.2733967674661105

neptune: 0.1394104549533083

According to this, my results are accurate.

\$\endgroup\$
8
8
\$\begingroup\$

Simplify the logic

It seems like:

if planet == "mercury":
    return number_of_days / days_in_year["mercury"]
elif planet == "venus":
    return number_of_days / days_in_year["venus"]
elif planet == "earth":
    return number_of_days / days_in_year["earth"]
elif planet == "mars":
    return number_of_days / days_in_year["mars"]
elif planet == "jupiter":
    return number_of_days / days_in_year["jupiter"]
elif planet == "saturn":
    return number_of_days / days_in_year["saturn"]
elif planet == "uranus":
    return number_of_days / days_in_year["uranus"]
elif planet == "neptune":
    return number_of_days / days_in_year["neptune"]

can be easily rewritten:

if planet in days_in_year:
    return number_of_days / days_in_year[planet]
raise Exception("Unknown Planet! Are you sure you've enter a planet?")

Also, one could consider keeping it even more simple and use the fact that:

return number_of_days / days_in_year[planet]

will throw the relevant exception for an invalid value.

Do not repeat yourself

You can try to avoid duplicated values and have a single source of information. In your case, the list of planet is indirectly hardcoded twice :

days_in_year = {"mercury": 88, "venus": 224, "earth": 365, "mars": 687, "jupiter": 4332, "saturn": 10759, "uranus": 30688,
                "neptune": 60182} #days in each planet's year

and

planets = ["mercury", "venus", "earth", "mars", "jupiter", "saturn", "uranus", "neptune"]

Maybe you could define a constant like DAYS_IN_YEAR_PER_PLANET (corresponding to your actual days_in_year dictionnary) and use it where you are using the list of the planets (for planet in DAYS_IN_YEAR_PER_PLANET: for instance).

Do not perform more operations than required

number_of_days = calculate_days(age) is computed for every planet in the list. It would be better from a performance point of view to feed the function a number of days.

Useless list (or useless test)

You are creating a list with values divisible by 4. Then you iterate over it and check if the value is divisible by 4. It seems like a waste of effort. Let's get rid of the list creation. Code is based on the initial version of your code.

def calculate_days(age):
    leap_days = 0 #holds number of leap days

    for j in range(age):
        if j % 4 == 0 and j % 100 != 0:
            leap_days += 1 #if leap years are divisible by four and not divisible by a hundred, add to leap days

    return (age * 365) - leap_days

Also, this can somehow be written by abusing the generator expression and the sum builtin:

def calculate_days(age):
    leap_days = sum(1
                    for j in range(age)
                    if j % 4 == 0 and j % 100 != 0)
    return (age * 365) - leap_days

Also, you could find mathematical expressions to compute the number of leap days in a more efficient way (constant time instead of linear time) but I'll keep this out of the review.

\$\endgroup\$
4
  • \$\begingroup\$ How can I only use values, or only the keys, of a dictionary? \$\endgroup\$ – CodesInTheValley Jul 13 '16 at 15:25
  • 1
    \$\begingroup\$ By default, it loops over keys. You have ways to iterate over (key, values) or over (values). More information here : stackoverflow.com/questions/3294889/… . \$\endgroup\$ – SylvainD Jul 13 '16 at 15:26
  • \$\begingroup\$ @cat This comment is more relevant to OP's question than to my answer. \$\endgroup\$ – SylvainD Jul 13 '16 at 21:15
  • \$\begingroup\$ I didn't notice that in the original code, now I see why it's an Exception and not a ValueError or something \$\endgroup\$ – cat Jul 13 '16 at 21:22

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.