9
\$\begingroup\$

I had a question about using the modulus operator in Python and whether I have used it in a understandable way.

This is how I've written the script:

#sum numbers 1 to 200 except mults of 4 or 7
def main():
    sum = 0
    for x in range(200+1):
        if (x % 4 and x % 7): #is this bad???
            sum = sum + x
    print sum


if __name__ == '__main__':
    main()

So my questions are:

  • Should I spell out the modulo more clearly? ie if x % 4 != 0 and x % 7 != 0
  • Should I be approaching this in a very different way?
\$\endgroup\$
  • 2
    \$\begingroup\$ Omitting the != 0 is fine, even preferable. Most everyone understands that non-zero numbers are True in most high-level languages, so you're not exploiting some tough-to-remember quirk that will trip you up later. \$\endgroup\$ – David Harkness Feb 16 '11 at 2:45
  • 2
    \$\begingroup\$ @David: "In most high-level languages"? I'd assume that the number of high-level languages in which this is not true is greater than the number of low-level languages in which it is not true. \$\endgroup\$ – sepp2k Feb 16 '11 at 6:07
  • \$\begingroup\$ @sepp2k I'm not sure I understand your comment. Could you explain further? \$\endgroup\$ – KyleWpppd Feb 16 '11 at 6:11
  • 1
    \$\begingroup\$ @Kyle: You make it sound as if 0 being false (and the other numbers being true) was a concept introduced by high-level languages (otherwise why say "most high-level languages" instead of "most languages"). This is not true - 0 being false was a concept introduced by machine language. And as a matter of fact I believe there are more high-level languages in which 0 is not usable in place of false than there are low-level languages in which 0 can not be used as false. \$\endgroup\$ – sepp2k Feb 16 '11 at 6:16
  • \$\begingroup\$ @ sepp2k, I got it now. I had originally submitted this code for a quick online test to qualify for an interview. And after I clicked submit I thought, "geez, that's probably a crappy way to make code someone else could read." I really wanted a feel for how more experienced programmers would look at it. Especially using the modulo in the way I did, that a remainder = True. \$\endgroup\$ – KyleWpppd Feb 16 '11 at 6:21
8
\$\begingroup\$

I think your use of % is fine, but that could be simplified to:

def main():
    print sum([i for i in range(201) if i % 4 and i % 7])

if __name__ == '__main__':
    main()

Edit: Since I had a bug in there, that's a pretty clear indication that the % is a tripwire. Instead, I'd probably do:

def divisible(numerator, denominator):
    return numerator % denominator == 0

def main():
    print sum(i for i in range(201) if not(divisible(i, 4) or divisible(i, 7)))
\$\endgroup\$
  • \$\begingroup\$ Ahhh... seeing it in a generator expression makes me feel like I should make the comparison explicit, as a relative n00b, it confuses me what the comparison is doing exactly. I think that's bad since it's my code. \$\endgroup\$ – KyleWpppd Feb 15 '11 at 23:39
  • \$\begingroup\$ What elegant looking code. \$\endgroup\$ – dreamlax Feb 16 '11 at 0:00
  • \$\begingroup\$ @munificent - The or should be and to skip numbers that are multiples of 4 and/or 7. I thought Kyle had it wrong at first, but after careful consideration I'm 93.84% sure it should be and. \$\endgroup\$ – David Harkness Feb 16 '11 at 2:50
  • \$\begingroup\$ Thanks Munificent! I think you answer helped me a lot in understanding generator expressions too. \$\endgroup\$ – KyleWpppd Feb 16 '11 at 3:40
  • \$\begingroup\$ @David - You're right. This is actually one of the things I don't like about how this uses %. i % 4 means "not divisible by four" which is backwards from what you expect (but also what we want, which makes it hard to read. \$\endgroup\$ – munificent Feb 16 '11 at 4:44
3
\$\begingroup\$

My program is the shortest possible:

print 12942

Use the formula of inclusion/exclusion.

There should be 200-(200/4)-(200/7)+(200/28) (Using integer division) = 200-50-28+7 = 129 terms in the sum.

The sum must be s(200) - 4*s(200/4) - 7*s(200/7) + 28*s(200/28) where s(n) = sum from 1 till n = n*(n+1)/2.

This evaluates to 0.5* (200*201 - 4*50*51 - 7*28*29 + 28*7*8) = 0.5*(40200 - 10200 - 5684 + 1568) = **12942**.

Why write a program if you can use math?

(I'll admit I used a calculator to multiply and add the numbers)

\$\endgroup\$
  • 2
    \$\begingroup\$ Because just doing the maths doesn't teach you anything about the programming language you're using. \$\endgroup\$ – dreamlax Feb 17 '11 at 5:54
  • \$\begingroup\$ @dreamlax I think that "your piece of code is more complex than required. Use a bit of math to simplify it" is a valid comment during code review. This is not learning-a-language.stackexchange.com. \$\endgroup\$ – Sjoerd Feb 17 '11 at 11:51
  • 2
    \$\begingroup\$ It's also not codegolf.stackexchange.com. We're reviewing the algorithm, not the result. \$\endgroup\$ – dreamlax Feb 17 '11 at 12:02
  • 1
    \$\begingroup\$ I don't find this solution very helpful, but in code as an algorithm it would be. \$\endgroup\$ – KyleWpppd Feb 17 '11 at 19:15
  • \$\begingroup\$ @KyleWpppd I don't know enough Python to turn this into correct Python code, that's why my answer has just one print statement. If anyone writes some Python code for the s() function, the integer divisions, and calling the s function, feel free to edit my answer. \$\endgroup\$ – Sjoerd Feb 19 '11 at 3:30
3
\$\begingroup\$

This is a more generalised version of the answers already given:

def sum_excluding_multiples(top, factors):
    return sum(i for i in range(top + 1) if all(i % factor for factor in factors))

print sum_excluding_multiples(200, (4, 7))
print sum_excluding_multiples(200, (3, 5, 10, 9))

This is @Sjoerd's answer as Python code:

def sum_excluding_multiples2(top, factor1, factor2):
    def sum_of_multiples(m=1):
        return m * int(top / m) * (int(top / m) + 1) / 2
    return (sum_of_multiples() - sum_of_multiples(factor1) 
        - sum_of_multiples(factor2) + sum_of_multiples(factor1 * factor2))

print sum_excluding_multiples2(200, 4, 7)

This is more complicated and harder to read, and I'm not sure how to generalise it to exclude multiples of more than two numbers. But it would be faster if very large numbers were involved, since it solves the problem mathematically instead of by iterating through a range.

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

I think x % 4 != 0 is clearer than x % 4, because:

  • The standard way to check if a number is divisible is x % 4 == 0. Of course that could also be written as not x % 4, but usually, it's not. x % 4 != 0 is more in line with the standard way of writing tests for divisibility.
  • x % 4 is probably more error-prone. Quoting munificient:

    'i % 4 means "not divisible by four" which is backwards from what you expect (but also what we want, which makes it hard to read.

It's not really a big deal though.


A few more comments on your code:

Don't shadow the built-in function sum() with your variable sum. Instead, use total or something similar.

You don't need the parentheses in if (x % 4 and x % 7):.

As has been mentioned in the comments, write sum = sum + x more concisely as sum += x.

I would make main() return the number and then write print main() in the last line. This way, you could reuse the function in a different context.

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