Improvements?

[coursecoder30879]

Hi,
I have written a program to display all twin primes less than 1,000. Could someone please read my code and give me ways to improve, for example: the spacing between code blocks, my choice of variable names, insert comments anywhere??, structure of the program, etc. Please don’t hold anything back. Anything that bothers you, please tell me. I’d really appreciate the advice and improvements. Thanks!

So, my program starts with prime1 which I assign 2 to. Then it enters a while loop and calls the function find_the_next_prime to find the next prime. So it starts with prime1 + 1 to check if it’s prime and keeps adding 1’s until it’s prime and returns that number. Then the rest I think is clear.

def is_twin_prime(prime1, prime2):
    return abs(prime1 - prime2) == 2


def find_next_prime(number):

    while True:
        is_prime = True
        for divisor in range(2, (number - 1) + 1):
            if number % divisor == 0:
                number += 1
                is_prime = False
                break

        if is_prime:
            return number


def main():
    prime_1 = 2

    while prime_1 < 1000:

        prime_2 = find_next_prime(prime_1 + 1)

        if is_twin_prime(prime_1, prime_2):
            print("({0:d},{1:d})".format(prime_1, prime_2))

        prime_1 = prime_2


main()

[ionatan]

- 1) + 1

Should probably not do that.

---

I think find_next_prime would be better written like:

import itertools

def find_next_prime(number):
    for n in itertools.count(number):
        if is_prime(n):
            return n

Because the nested loop, break, return, variables – it’s really hard to see when it’s going to stop and what’s actually being increased. The loop logic is all over the place, literally every line is part of controlling the looping.

---

Generally less blank lines inside functions. Use them for logical groups, default to no blanks if unsure.

---

The string formatting, you can omit the index (0 and 1), like: {:d} and actually, the d isn’t required either because the default string formatting for int is already exactly that, so you can have just {}
And if you’re using 3.6+ you can put an f in front of the string and skip the format method:
f'({prime_1},{prime_2})'

---

There’s a tool called pylint that you can run on your code. It won’t have very much to say about this particular code though, except for docstrings (you can and should ignore things you don’t agree with, docstrings would be very overkill here)

[coursecoder30879]

So, instead of (number - 1) + 1, I should just put number. I was thinking that range(a, b) omits b. So it only goes from a to b-1. So I thought putting a + 1 after b to become range(a, b + 1). So, when you have a range [0, 100] (includes the endpoints) and it goes up by 10. Should I write it as range(0, 100 + 1, 10) or range(0, 100 + 10, 10) or …?

[ionatan]

You’re doing
x + 1 - 1
just write x
Or if you do mean inclusive, which you don’t because all numbers are divisible by themselves, you’d put + 1

[coursecoder30879]

Sorry, one more thing. What do you mean by logical groups. Do you mean like if and else?

[ionatan]

No I mean like “these lines are doing one thing”

This is really up to you, but I definitely think you should use less of them, or rather, default to not having them and only add them when you think it’s needed.

It’s kind of breaking my flow of reading, if that makes sense. It’s. Like. Putting. A. Full. Stop. Between. Each. Word.

[ionatan]

I’m not going to claim this is an improvement, but an alternative perhaps.

I like to separate generating and checking/processing things. Your loop in main is doing both.
What if you generated all consecutive pairs instead, and iterated over that?

def consecutive_primes():
    b = 2
    while True:
        a, b = b, next_prime(b)
        yield a, b


for prime_1, prime_2 in consecutive_primes():
    if twin....

…At this point I’m probably just playing around. This might fall into the category of getting getting fancy for little to no good reason, overall not an improvement.

(Also, I would like next_prime to exclude the number given to it, because it does say next, right.)

Actually. There is an improvement here. The loop generating prime pairs

while True:
    a, b = b, next_prime(b)

Is nicer than what’s in your main, it puts everything in one place. (spreading out loop logic is kinda bad)

And, there’s a bug here. b may be less than 1000, but the next one might not be. (It may produce the correct numbers, but that would be coincidence) (Actually the bug is in your code, because this code doesn’t (yet) stop at 1000)

[coursecoder30879]

Thank you for spending your time to help me. I appreciate it. How’s this? In general, is it wrong to use while True?

import sys


def is_prime(number):
    for divisor in range(2, number):
        if number % divisor == 0:
            return False
    return True


def is_twin_prime(prime1, prime2):
    return abs(prime1 - prime2) == 2


def main():
    prime1 = 2
    number = 3

    while True:
        while not is_prime(number):
            number += 1
        if prime1 >= 1000 and number >= 1000:
            sys.exit()
        if is_twin_prime(prime1, number):
            print(f"({prime1},{number})")
        prime1, number = number, number + 1


main()

[ionatan]

sys.exit is not something you should use here, that’s for communicating a status code to the operating system… it’s not a break statement

No, nothing wrong with while True.

The goal is to make each part of the code obvious.

I really want to get rid of the nested loop:

prime2 = 2
while True:
    prime1, prime2 = prime2, next_prime(prime2)
    if prime2 >= 1000:
        break

    if is_twin_prime(prime1, prime2):
        print(...)

But I still don’t like that. I’d rather see what’s done with each pair:

for prime1, prime2 in consecutive_primes(1000):
    if is_twin_prime(prime1, prime2):
        print(...)

And have the generation of those pairs be placed in its own location:

def consecutive_primes(upper_bound):
    '''(2, 3), (3, 5), (5, 7), ...'''
    a, b = 2, 3
    while b < upper_bound:
        yield a, b
        a, b = b, next_prime(b)

That way each part is neat. Each part does one thing and one only. In the context of what they do, it is obvious how they do it. And there’s only one context, only one thing being done.

I feel like I’m just waving a magical wand where everything in the world is trivial when I get these things right. There’s always going to be tradeoffs. Breaking things out moves part of the code AWAY, making the whole harder to see. But if you broke out a piece where the details are not important, then you made it much easier to read and you can treat them completely separately.

…You’re entirely welcome to disagree with what looks good here. There’s an artistic element to this, nobody can truly tell you you’re doing it wrong, unless you’re painting with faeces cause, well, most will draw a line there, or way earlier perhaps.

[ionatan]

You want to look at any piece of code and understand it all at once.

This means avoiding double-negatives, spread out loop logic, lots of nesting, lots of different flow control paths.

It’s like how an english sentence should be somewhat short or you lose track of what’s being said.
“A piece of code” could be a whole function or something you’ve decided to put blank lines around.

I really have no clue if this is something that can be learned quickly or takes lots of time. Developing “good taste” (whatever that is) certainly can take time. Aim for obvious, and wait for experience to kick in.

Sorry. This isn’t very succinct at all. You’re kinda just getting my thoughts as they come.