7
\$\begingroup\$

My code prints the next 20 leap years:

def loop_year(year):
    x = 0
    while x < 20:
        if year % 4 != 0 and year % 400 != 0:
            year +=1
            ##print("%s is a common year") %(year)
        elif year % 100 != 0:
            year +=1
            print("%s is a leap year") % (year)
            x += 1


loop_year(2020)         

I want to use fewer lines. I thought of using a for but some people said it was a waste of time. What else can I do?

\$\endgroup\$
  • 2
    \$\begingroup\$ Why are you concerned about the amount of lines of code? Performance and readability should come first. Lines of code are quite irrelevant in Python. \$\endgroup\$ – Mast Apr 13 '16 at 11:34
  • \$\begingroup\$ Hi @Mast, I care because there is certain lack of Elegance that is lost, in code or in whatever you want actually. Call it Poetry, waste of time or sophistry, take your pick, I like doing less to get more. This is the why. \$\endgroup\$ – Andy K Apr 13 '16 at 13:04
  • 1
    \$\begingroup\$ If year divided by 4 is different from 0, then we already know that year divided by 400 is different from 0. ´year%400´ should be as an or statement in the elif, ´or year % 400==0´. This is slightly irrelevant, since calendar.isleap is the correct answer. \$\endgroup\$ – Taemyr Apr 13 '16 at 14:25
  • 1
    \$\begingroup\$ How few lines do you want to use? In some cases, more lines of code can mean better application performance, and better code readability. If you're just looking for fewer lines, you could look into Code golf: codegolf.stackexchange.com/a/50891/19451 \$\endgroup\$ – Clark Kent Apr 13 '16 at 16:12
  • 1
    \$\begingroup\$ Let's say we have year to store the current year. To get the next leapyear, just add year % 4 to year, print the vakue out, then with a for loop, each iteration increase the year count with 4, ao you always get a leap year. It's 5-6 lines of code if you ask me. \$\endgroup\$ – Bálint Apr 14 '16 at 17:12
11
\$\begingroup\$

I can't see a way to make this shorter (I can't see how is this relevant to any performance issue in Python) as your solution is straight forward but I have some advice:

Use calendar module to get rid of those ugly conditions.

calendar.isleap(year)

Formatting:

Try formatting your strings using format() (it's much cleaner looking):

This:

print("%s is a leap year") % (year)

Would become this:

print('{} is a leap year!'.format(year))

In your method, add another parameter which will be the number of years (this will make your code more user-friendly) - you don't want somebody to modify the number of leap years inside your block code. Just make it happen when they are calling the method:

def loop_year(year, number_of_years):
    ...

More, your variable names should have specific names based on their usage: I would personally change x to leap_year_counter. When it comes to a simple counter, your naming solution it's also ok, but if your code will become more robust, you'll have a hard time figuring what is what.

IF/ELSE Statements:

As others mentioned, you have year += 1 in your both statements, so just move it after them. More, in this case, you can also remove the else statement.

# ...
if calendar.isleap(year):
    print('{} is a leap year!'.format(year))
    leap_year_counter += 1
year += 1
# ...

Finally, your method will look like this:

import calendar


def loop_year(year, number_of_years):
    leap_year_counter = 0
    while leap_year_counter < number_of_years:
        if calendar.isleap(year):
            print('{} is a leap year!'.format(year))
            leap_year_counter += 1
        year += 1
loop_year(2016, 20)
\$\endgroup\$
9
\$\begingroup\$

First of all, a review.

x = 0

I would give a more meaningful name.

year +=1

PEP 8 says:

Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).

(Emphasis mine)

if ...:
    ...
    year += 1
else:
    ...
    year += 1

If you are doing the same thing regardless of the if statement, move that line after the if and else.

print("%s is a leap year") % (year)

% format strings are not officially deprecated, but it is recommended to use .format() Yours also isn't quite how you want it. You'll need to move the first close parenthesis to after the second one. Right now you are running % on the call to print and (year) instead of calling print() with the result of running % on a string and (year).


Now for the better ways.

There's a fancy little function that was made for checking leap years: calendar.isleap

from calendar import isleap

def loop_year(year):
    found = 0
    while found < 20:
        if isleap(year):
            print("{} is a leap year".format(year))
            found += 1
        else:
            print("{} is a common year".format(year))
        year += 1

If you don't care about the common years (based on your commenting out that line), remove the else:

from calendar import isleap

def loop_year(year):
    found = 0
    while found < 20:
        if isleap(year):
            print("{} is a leap year".format(year))
            found += 1
        year += 1
\$\endgroup\$
5
\$\begingroup\$

Here is how I would have done it:

def print_leap_years(year):
    for y in range(year, year + 20 + 1):
        if y % 4 == 0 and (y % 100 != 0 or y % 400 == 0):
            print("{} is a leap year".format(y))

My solution has the following advantages:

  • It properly handles years that are multiples of 100. Multiples of 100 are not leap years, unless they are multiples of 400. For example, 2000 is a leap year, while 2100 is not.
  • I am using the range function to make the code smaller and avoid the x = 0 and x += 1 statements.
  • The name print_leap_years is, IMO, more descriptive than loop_year.
  • I am using the format method of the string type, which I think is a bit easier to read. Keep in mind that this won't work with Python versions earlier than 2.6.

If you want to make it even simpler and avoid reinventing the wheel, you should use the calendar.isleap function:

def print_leap_years(year):
    for y in range(year, year + 20 + 1):
        if calendar.isleap(y):
            print("{} is a leap year".format(y))

Following @Dex' ter's suggestion and making the number of years a parameter, the function would look like this:

def print_leap_years(year, year_count):
    for y in range(year, year + year_count + 1):
        if calendar.isleap(y):
            print("{} is a leap year".format(y))

and be called like print_leap_years(2016, 20). I think year_count is a bit easier to read and less verbose than number_of_years, but that may be a matter of personal taste.

I think that producing the sequence of years and printing them should be two separate things, in case you want to get the list and do something with it other than printing it. You can easily produce the sequence with a list comprehension:

def get_leap_years(year, year_count):
    return [y for y in range(year, year + year_count + 1) if calendar.isleap(y)]

and print it with print(get_leap_years(2016, 20)), or if you want the output to look exactly like the above versions, print("\n".join(get_leap_years(2016, 20)) + "\n"). This is probably my favorite out of all of these attempts.

If you are trying to produce a huge number of years, it may be somewhat wasteful to keep them all in memory at once. You can use a generator expression to return an iterator that produces the years one at a time, as needed:

def get_leap_years(year, year_count):
    return (y for y in range(year, year + year_count + 1) if calendar.isleap(y))

However, this will be a bit trickier to work with. If you try to print it directly, you won't get anything meaningful. You would have to convert it to a list first: print(list(get_leap_years(2016, 20))). If you want to index the sequence like a list (for example years[5]), slice the sequence (years[5:10]), or iterate over it more than once for any reason, then you would also have to convert it to a list or maybe use itertools.islice. Printing it separated by line breaks can still be done just like with the list: print("\n".join(get_leap_years(2016, 20)) + "\n").

EDIT: @scottbb pointed out that the problem was to print the next 20 leap years, not the leap years within the next 20 years. This can be done as follows:

from calendar import isleap
from itertools import count, islice

def get_leap_years(year, year_count):
    return islice((y for y in count(start=year) if isleap(y)), year_count)

Keep in mind that islice returns an iterator, so you may need to convert the result to a list, depending on your needs.

\$\endgroup\$
  • \$\begingroup\$ Hi, welcome to Code Review! This is a very good first answer, kudos! \$\endgroup\$ – Tunaki Apr 13 '16 at 16:45
  • 4
    \$\begingroup\$ You changed the output/intent of the OP's problem. The OP's code prints the next 20 leap years, not the leap years in the next 20 years. \$\endgroup\$ – scottbb Apr 13 '16 at 18:52
  • \$\begingroup\$ Oops. I somehow didn't notice that. I will fix this when I get a chance. \$\endgroup\$ – Elias Zamaria Apr 13 '16 at 19:35
  • \$\begingroup\$ Hi @EliasZamaria, many thanks for your answer. I learnt a few things today. :) \$\endgroup\$ – Andy K Apr 14 '16 at 8:39
5
\$\begingroup\$

You loop through every year, but you know a priori you only need to test every 4 years. You can reduce your loop overhead by a factor of 4.

To do so, first you need to "precondition" the input year so that it starts on the next (or current) leap year. You want to add 0, 3, 2, or 1, depending on whether year % 4 is 0, 1, 2, or 3. Either of the following methods will do that:

  • year += abs(year % -4)
  • year += (4 - (year % 4)) % 4

Then, in your loop, remove all instances of year+=1 as suggested by others. Instead, at the bottom of the loop, use year += 4. Because of this, you can remove checks for year % 4 != 0 in your loop. Your algorithm would be reduced the following

def loop_year(year, leapyear_count):
    year += abs(year % -4)
    while leapyear_count > 0:
        if year % 100 != 0 or year % 400 == 0:
            print("%s is a leap year") % (year)
            leapyear_count -= 1
        year += 4

Of course, as suggested by others, your conditional could just be if calendar.isleap(year): instead. However, calendar.isleap() will check first for year % 4 == 0, which will always be true. So from a pure performance issue (i.e., if you were generating huge list of the next N leap years), the way it is written above would be more performant.

Update: Claims of performance should be backed up with measurements. I liked Joe Wallis's answer using itertools.islice() and itertools.count() so much, I combined his approach with mine and timed them.

The functions:

from calendar import isleap
from itertools import count, islice

def leep_years_from(year, amount=20):
    return islice((y for y in count(year) if isleap(y)), amount)

def leap4_isleap(year, amount=20):
    return islice((y for y in count(year + abs(year % -4), 4)
                   if isleap(y)), amount)

def leap4(year, amount=20):
    return islice((y for y in count(year + abs(year % -4), 4)
                   if year % 100 != 0 or year % 400 == 0), amount)

All three functions, leep_years_from(), leap4_isleap(), and leap4() do the same thing. leep_years_from() iterates over every year; the other 2 iterate over every 4 years, but leap4() doesn't call calendar.isleap() to determine if the year is a leap year.

Here is one set of results on my machine running cProfile.run() for all three functions, finding the next 10 million leap years (I've eliminated a few unimportant and blank lines):

>>> cProfile.run('list(leep_years_from(2000,10000000))')
        51237117 function calls in 18.425 seconds

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <stdin>:1(leep_years_from)
 10000001    9.659    0.000   17.067    0.000 <stdin>:2(<genexpr>)
        1    1.358    1.358   18.425   18.425 <string>:1(<module>)
 41237113    7.408    0.000    7.408    0.000 calendar.py:97(isleap)

>>> cProfile.run('list(leap4b(2000,10000000))')
         20309284 function calls in 6.991 seconds

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <stdin>:1(leap4b)
 10000001    3.259    0.000    5.678    0.000 <stdin>:2(<genexpr>)
        1    1.312    1.312    6.991    6.991 <string>:1(<module>)
 10309279    2.419    0.000    2.419    0.000 calendar.py:97(isleap)

>>> cProfile.run('list(leap4(2000,10000000))')
         10000005 function calls in 3.422 seconds

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <stdin>:1(leap4)
 10000001    2.037    0.000    2.037    0.000 <stdin>:2(<genexpr>)
        1    1.385    1.385    3.422    3.422 <string>:1(<module>)

The change from checking if every year is a leap year to only checking if every 4 years is a leap year cut the execution time by a factor of 3. By eliminating the check if every 4th year is a leap year (the first check that occurs in calendar.isleap()) and the overhead of the isleap() function call, another factor 2 is gained.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for liking my answer, yours is quite nice too! Just wanted to let you know you didn't time your original loop_year. I changed the print(... year) to yield year and it was faster than leap4, by a somewhat significant amount. (leap4: 26.83, loop_year: 25.05, ~7% speed up) IMO the clarity from itertools is worth the 7%. \$\endgroup\$ – Peilonrayz Apr 13 '16 at 23:13
  • 1
    \$\begingroup\$ @JoeWallis 100% agreed, and I think you win the OP's original intent (short code). You're right, I didn't time it. I just assumed it would be slower (that's what I get for assumptions). Also goes to show that when it comes to performance and optimization, intuition can often lead one astray. IMO, nominal speedup (say, less than 10%) is only justified for code that is either critical real-time, or is profiled as frequent execution in a tight loop. But 2x–6x speedup, and readability to boot? Sign me up! =) \$\endgroup\$ – scottbb Apr 13 '16 at 23:20
  • 1
    \$\begingroup\$ Of course, if you really do have a reason to care about speed, a different language may be worth considering. A quick test shows that generating 10 million leap years in C++ takes around 23 milliseconds. \$\endgroup\$ – Jerry Coffin Apr 14 '16 at 13:19
  • \$\begingroup\$ @JerryCoffin +1. I'd restate that as "if you really do have a reason to prioritize speed". There's always good reason to care about speed in the sense of not being wasteful or "lazy" algorithmically. \$\endgroup\$ – scottbb Apr 14 '16 at 15:07
  • \$\begingroup\$ @scottbb: Fair enough. \$\endgroup\$ – Jerry Coffin Apr 14 '16 at 15:35
3
\$\begingroup\$

That's a very simple comment, but you could do the year += 1 outside of your if/else if conditions. You can also always try to write your conditional commands (in the if loops) as one line, right after the if (condition) : but then the code gets heavier to read, in my opinion.

def loop_year(year):
    x = 0
    while x < 20 :
        if year % 4 != 0 and year % 400 != 0:
            ##print("%s is a common year") %(year)
        elif year % 100 != 0:
            print("%s is a leap year") % (year)
            x += 1
        year += 1
loop_year(2020)
\$\endgroup\$
3
\$\begingroup\$

You can make a one-line function to make a list of leap years. This uses a more 'functional' / iterator spin on your algorithm.

Using calendar.isleap(year) just as @Dex'ter does, you can also use itertools.count and itertools.islice. These will remove your += 1s that 'litter up' the program.
You may also want to split your program into a leap years generator, and a display for those numbers.
This gives you some nice small code:

from calendar import isleap
from itertools import count, islice

def leep_years_from(year, amount=20):
    return islice((y for y in count(year) if isleap(y)), amount)

for leap_year in leep_years_from(2020):
    print("{} is a leap year".format(leap_year))

Also your algorithm incorrectly says that 2000 is not a leap year, and actually breaks (infinitely loops) on that year. This is as you use the following statement to check if the year is a leap year:

not (year % 4 != 0 and year % 400 != 0) and year % 100 != 0

Where calendar.isleap uses:

year % 4 == 0 and (year % 100 != 0 or year % 400 == 0)
\$\endgroup\$
2
\$\begingroup\$

Your code doesn't print the next 20 leap years.

At the time of my posting, your code is as follows:

def loop_year(year):
    x = 0
    while x < 20:
        if year % 4 != 0 and year % 400 != 0:
            year +=1
            ##print("%s is a common year") %(year)
        elif year % 100 != 0:
            year +=1
            print("%s is a leap year") % (year)
            x += 1

The necessary conditions for a year to be a leap year are:

  1. year is divisible by 4
  2. year is divisible by 400 or not divisible by 100

The line:

if year % 4 != 0 and year % 400 != 0:

is redundant. Any year that is not divisible by 4 is not divisible by 400, since 400 is divisible by 4. All this condition does is skip over years that are not divisible by 4.

The next condition, which is checked only if year % 4 == 0, checks if year % 100 != 0.

So we execute the code:

        year +=1
        print("%s is a leap year") % (year)
        x += 1

when

  1. year is divisible by 4
  2. year is not divisible by 100

You need to change the condition so that when year is divisible by 400, we execute the next few lines of code. You could change it to:

    elif year % 100 != 0 or year % 400 == 0:

Then, you execute:

        year +=1
        print("%s is a leap year") % (year)
        x += 1

At this point, year should be a leap year, but you increment it before printing, so you actually print (leap year + 1) for each leap year. You can fix this incrementing after printing.

        print("%s is a leap year") % (year)
        year += 1
        x += 1

After these changes, your code would look like:

def loop_year(year):
    x = 0
    while x < 20:
        if year % 4 != 0:
            year +=1
            ##print("%s is a common year") %(year)
        elif year % 100 != 0 or year % 400 == 0:
            print("%s is a leap year") % (year)
            year += 1
            x += 1

But you still have a problem: when year is divisible by 100 but not by 400, you never increment it so you enter an infinite loop. To fix this, you should increment year outside of your conditionals. Since you increment year regardless of whether or not year is divisible by 4, you can combine your conditional statements, leaving:

def loop_year(year):
    x = 0
    while x < 20:
        if year % 4 == 0 and (year % 100 != 0 or year % 400 == 0):
            print("%s is a leap year") % (year)
            x += 1
        year += 1

Now this works, but it could still be cleaner and faster. Instead of checking all integers above year, you can just check all year divisible by 4. You can simplify even further by using libraries. Here is how I would implement this:

from itertools import count, islice
from calendar import isleap

def leapyears(start):
    start += (4 - (start % 4)) % 4
    for leap in (year for year in count(start, 4) if isleap(year)):
        yield leap

def loop_year(start):
    for leap in islice(leapyears(start), 20):
        print "{} is a leap year".format(leap)

Here, you make an infinite generator of leap years, then just take the first 20 values and print them.

If you just want to use fewer lines of code and don't care at all about readability, you can use a similar approach:

from itertools import count, islice
def loop_year(s):
    for l in islice((y for y in count(s+(4-s%4)%4,4) if y%100!=0 or y%400==0),20):
        print "{} is a leap year".format(l)

But definitely don't do that if you're not golfing.

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