10
\$\begingroup\$

I made a function that takes an argument x and returns True if x is a prime number and False if x is a non-prime number.

I am a beginner and think the code can be improved. I had to add the else statement because for is_prime(2), I kept getting None instead of True. I think this was because when x is 2, list_n = range(2,2) prints [].

Are there ways I could change this so there is not a separate else statement?

def is_prime(x):
    list_n = range(2,x)

    if x <= 1:
        return False
    elif x-1>2:
        for n in range(len(list_n)):
            list_n[n] = x % list_n[n]
        for n in list_n:
            if n == 0:
                return False
        for n in list_n:
            if n > 0:
                return True
    else:
        return True
\$\endgroup\$
12
\$\begingroup\$

To begin with, you don't need list_n at all: a decision that the number is composite can be made on the fly, as soon as some remainder becomes 0. If none of them were 0, you know that the number is prime:

    for n in range(2, x):
        if x % n == 0:
            return False
    return True

is effectively what your code is doing, but without the list, and without special case for 2.

That said, of course the algorithm could be greatly improved, but it is beyond the point of this exercise.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Don't forget the edge case n <= 1, which was handled in original code. \$\endgroup\$ – user3447239 Jun 18 '17 at 15:41
  • \$\begingroup\$ n.b. that for n in range(2, x) checks more numbers than necessary to see if x is prime. The greatest unique divisor of x (that isn't a co-divisor of a smaller number, e.g. 32 => (2, 16)) is sqrt(x). for n in range(2, int(x ** 0.5)+1) is sufficient to test for primality. \$\endgroup\$ – Adam Smith Jun 18 '17 at 17:08
  • 1
    \$\begingroup\$ @AdamSmith Yes, this is what the last line in my post is about. \$\endgroup\$ – vnp Jun 18 '17 at 17:10
  • \$\begingroup\$ @vnp Ah. I assumed you were talking about using a Sieve or etc \$\endgroup\$ – Adam Smith Jun 18 '17 at 17:13
7
\$\begingroup\$
  1. You don't need to create a list. Just iterate over potential divisors:
    for d in xrange(2, x): ...

  2. The condition x - 1 > 2 seems kind of complicated. I suggest adding a guard at the beginning of the function:

    if n <= 1:
        return False
    

    and using a generic procedure for all other numbers (including 2. It works for x = 2. There's nothing special about it). It'll make your code easier to follow.

  3. You can make it more efficient by checking the divisors only up to sqrt(x) inclusively. If a number is not a prime, it must have at least one divisor not greater then its root.

\$\endgroup\$
3
  • \$\begingroup\$ Your first point depends on whether he's using Python 2 or Python 3. Python 3 has no xrange, since it's range already behaves as such. \$\endgroup\$ – Mast Jun 18 '17 at 17:50
  • \$\begingroup\$ @Mast He must be using python 2. Otherwise, list_n[n] = ... won't work (range doesn't support assignment in python 3) and his code is broken for any x >= 3. \$\endgroup\$ – kraskevich Jun 18 '17 at 17:58
  • \$\begingroup\$ I'm so accustomed to Python 2 I didn't even catch that would break on 3. Good call. \$\endgroup\$ – Mast Jun 18 '17 at 18:00
5
\$\begingroup\$
  1. Use a better variable name than x.
  2. Add separate if-clauses to check if your input number is <= 1, == 2 and % 2 == 0.
  3. Beyond the above exclusions, you only have to deal with odd numbers. Check only against odd divisors: xrange(3, sqrt(your_input), 2).
  4. This loop is entirely useless:

    for n in list_n:
        if n > 0:
            return True
    

You'll end up with something like

def is_prime(integer):
    """Returns True if input is a prime number"""
    if integer < 2 or not isinstance(integer, (int, long)):  # remove long if using Python 3
        return False
    if integer == 2:
        return True
    if integer % 2 == 0: # which can also be written as -> if not integer & 1:
        return False
    for divisor in xrange(3, int(math.sqrt(integer)) + 1, 2):
        if integer % divisor == 0:
            return False
    return True
\$\endgroup\$
3
  • 8
    \$\begingroup\$ I must say I disagree with this. Calling a variable x where it is clear that this is an abstract variable (yes, an integer) is much more useful than calling it integer. If you see a function called add(a, b) isn't it obvious what a and b do? \$\endgroup\$ – Daniel Jun 17 '17 at 21:34
  • \$\begingroup\$ @Coal_ yes, but nearly all pep8 based linters will report that as an invalid argument name. \$\endgroup\$ – hjpotter92 Jun 18 '17 at 5:23
  • 2
    \$\begingroup\$ @hjpotter92 then fix the linter. single-letter variable names are made for instances like this, and PEP8 does not declare against them. Sounds like an overreach of the linter. \$\endgroup\$ – Adam Smith Jun 18 '17 at 17:14
3
\$\begingroup\$

Just remove else: and unindent return True. That should work since if and elif will always return if entered.

\$\endgroup\$
3
\$\begingroup\$

The third for loop can be replaced with a single return True statement (because it's the exact opposite of the second for). Also, the else: can be removed as if any previous conditions were satisfied, the function would have already returned. The return True block formerly in the else: block can be fused with the third for loop's return, producing this code:

def is_prime(x):
    list_n = range(2,x)

    if x <= 1:
        return False
    elif x-1 > 2:
        for n in range(len(list_n)):
            list_n[n] = x % list_n[n]
        for n in list_n:
            if n == 0:
                return False
    return True

Another suggestion is to replace the first for with a list comprehension. This is left as an exercise to the reader.

\$\endgroup\$
0
\$\begingroup\$

Below code should be enough to test if a number is prime or not.

num = int(input("Please enter a number to check if its prime : "))

def primeNumber():
    if (num <= 1):
        return False

    for i in range(2,num):
        if(num % i == 0):
            return False

    return True

if (primeNumber() == True):
    print(f"{num} is a prime number")
else:
    print(f"{num} is not a prime number")
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! However, please explain why this solution is better than the one in the question. \$\endgroup\$ – Null Dec 17 '18 at 16:03

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.