1
\$\begingroup\$

I'm looking for general feedback as how to improve my code. I wrote the program starting out with no functions, making it very difficult to adapt. When I separated the program into functions it became very repetitive.

How can I improve my code?

"""
A program for comparrison of triangulation (wave or skipping stones, not sure what to call it?) to division by same limited known prime numbers
as how well they spot new ones
"""

    from math import sin #only for wave
    from math import pi #period and period displacement of wave
    from math import sqrt #On the top of my head estimate of average overlapping multiplied waves. sin(pi/4) = sqrt(2)/2
    from math import e  
    from statistics import mean
    def Primer(number, numbers):#Generates primes by modulus for every prime in numbers
        iteration = 0
        while number % numbers[iteration] != 0: # as long as number modulus prime givesw remainder it keeps dividing
            iteration +=1
            if iteration == len(numbers):# Every former prime in list of primes hass been divided with remainder
                return number
            else:
                pass
    def Triangulation(numbers, x): #or skipping stone analogy. I dont understand this in combinatorics. prime 2 wave skips every other, prime every third and so on
        wave = 1
        for number in numbers:
            wave *= abs(sin(x*pi/number)*2/sqrt(2)) # sqrt(2)/2 * 2/swrt(2) = 1 to keep the wave around zero
        if wave > 1:
            return wave, x
        else:
            return "","" #cant unpack None type
    primes = [2,3]
    exponent = 3
    limit = int(10**exponent)
    window = [0, limit]
    triangulation = []
    true = []
    false = []
    primes_for_triangulation = []
    use_primes_up_to = e**(exponent)
    primes_in_window = []
    wave_amplitude = []
    true_wave_amplitude = []
    false_wave_amplitude = []
    primes_by_limited_division = []
    false_quotient = []
    true_quotient = []
    for number in range(len(primes)+2, limit, 1):
        possibly_prime = Primer(number, primes)#feeds primer with numbers and past primes...
        if type(possibly_prime) == int:
            primes.append(possibly_prime)#when prime is found it gets put into cycles
    for prime in primes:#creating a list from part of list
        if prime < use_primes_up_to:
            primes_for_triangulation.append(prime)
        else:
            pass
        if prime >= window[0] and prime < window[1]:#remenant from when window was different than zero to limit
            primes_in_window.append(prime)
        else:
            pass
    for x in range(window[0], window[1], 1):# For every x down the number line the skipping stones hops over unkown primes diving into previous ones...
        possibly_wave_amplitude, possibly_triangulated = (Triangulation(primes_for_triangulation, x))
        if type(possibly_triangulated) == int: #... if its int theyre in the air
            triangulation.append(possibly_triangulated)
            wave_amplitude.append(possibly_wave_amplitude)
    for tri in range(0, len(triangulation), 1):#seperating 2 list into 4 by true or false
        if triangulation[tri] in primes:
            true.append(tri)
            true_wave_amplitude.append(wave_amplitude[tri])
        else:
            false.append(tri)
            false_wave_amplitude.append(wave_amplitude[tri])
    for number in range(4, limit, 1): #looking for primes...
        limited_division = Primer(number, primes_for_triangulation)#...by limited prime list
        if type(limited_division) == int:
            primes_by_limited_division.append(limited_division)
    for possibly_prime in primes_by_limited_division:#seperating a list into 2 by true or false
        if possibly_prime in primes:
            true_quotient.append(possibly_prime)
        else:
            false_quotient.append(possibly_prime)
    #print out dosent handle zero
    print("LIMITED DIVISION AND TRIANGULATION UP TO LIMIT", limit)
    print("Primes up to ", use_primes_up_to, " were used to triangulate and to do limited division")
    print("How many limited true divided ", len(true_quotient))       
    print("How many limited faulty divided ", len(false_quotient))
    print("True per false ", len(true_quotient)/len(false_quotient))
    print("Triangulate in window with lower bound being ", window[0], " and upper bound being limit ", window[1])
    print("How many triangulated to be primes = ", len(triangulation))
    print("Triangulated per window span = ", len(triangulation)/(window[1]-window[0]))
    print("How many triangulated per primes in window ", len(triangulation)/len(primes_in_window)) #Zero division
    print("How many Falesly triangulated ", len(false))
    print("How many truely triangulated ", len(true))
    print("True per false ", len(true)/len(false)) #Zero division
    print("Mean of true wave amplitude ", mean(true_wave_amplitude))
    print("Mean of false wave amplitude ", mean(false_wave_amplitude))
    print("Largest true wave amplitude ", max(true_wave_amplitude))
    print("Largest false wave amplitude ", max(false_wave_amplitude))
    print("Smallest true wave amplitude ", min(true_wave_amplitude))
    print("Smallest flase wave amplitude ", min(false_wave_amplitude))
    print("\n Wave amplitude of falsely triangulated \n\n", false_wave_amplitude)
    print("\n Wave amplitude of truely triangulated \n\n", true_wave_amplitude)

I would be specifically interested in:

  • How to slim down code by handling lists in other ways than with the for statement.

    for tri in range(0, len(triangulation), 1):#seperating 2 list into 4 by true or false
        if triangulation[tri] in primes:
            true.append(tri)
            true_wave_amplitude.append(wave_amplitude[tri])
        else:
            false.append(tri)
            false_wave_amplitude.append(wave_amplitude[tri])
    
  • How to handle returning None and a tuple of two strings?

    def Triangulation(numbers, x): #or skipping stone analogy. I dont understand this in combinatorics. prime 2 wave skips every other, prime every third and so on
        wave = 1
        for number in numbers:
            wave *= abs(sin(x*pi/number)*2/sqrt(2)) # sqrt(2)/2 * 2/swrt(2) = 1 to keep the wave around zero
        if wave > 1:
            return wave, x
        else:
            return "","" #cant unpack None type
    
  • How to not fill list with None type, or how to remove None type easily?

    for number in range(4, limit, 1): #looking for primes...
        limited_division = Primer(number, primes_for_triangulation)#...by limited prime list
        if type(limited_division) == int:
            primes_by_limited_division.append(limited_division)
    
\$\endgroup\$
3
\$\begingroup\$

Well, your biggest problem is the length of the scope of variables. You declare/define all the variables at the beginning before all the code. This is not required, nor is it any good. When I try to read the code I have no chance to find the usage of variables as I have to scan all(!) lines if it is altered somewhere. Such long scope is real evil. As you said the code is unreadable and thus unmaintainable. your variable names get long and longer to avoid name collisions. So we start to move the variable definitions down in the code to the place where they are needed first.

# [...]

primes = [2,3]
exponent = 3
limit = int(10**exponent)
for number in range(len(primes)+2, limit, 1):
    possibly_prime = Primer(number, primes)     # feeds primer with numbers and past primes...
    if type(possibly_prime) == int:
        primes.append(possibly_prime)           # when prime is found it gets put into cycles

window = [0, limit]
primes_for_triangulation = []
use_primes_up_to = e**(exponent)
primes_in_window = []
for prime in primes:#creating a list from part of list
    if prime < use_primes_up_to:
        primes_for_triangulation.append(prime)
    else:
        pass
    if prime >= window[0] and prime < window[1]:#remenant from when window was different than zero to limit
        primes_in_window.append(prime)
    else:
        pass

triangulation = []
wave_amplitude = []
for x in range(window[0], window[1], 1):# For every x down the number line the skipping stones hops over unkown primes diving into previous ones...
    possibly_wave_amplitude, possibly_triangulated = (Triangulation(primes_for_triangulation, x))
    if type(possibly_triangulated) == int: #... if its int theyre in the air
        triangulation.append(possibly_triangulated)
        wave_amplitude.append(possibly_wave_amplitude)

true = []
false = []
true_wave_amplitude = []
false_wave_amplitude = []
for tri in range(0, len(triangulation), 1): # seperating 2 list into 4 by true or false
    if triangulation[tri] in primes:
        true.append(tri)
        true_wave_amplitude.append(wave_amplitude[tri])
    else:
        false.append(tri)
        false_wave_amplitude.append(wave_amplitude[tri])

primes_by_limited_division = []
for number in range(4, limit, 1):                                   # looking for primes...
    limited_division = Primer(number, primes_for_triangulation)     # ...by limited prime list
    if type(limited_division) == int:
        primes_by_limited_division.append(limited_division)

false_quotient = []
true_quotient = []
for possibly_prime in primes_by_limited_division:   # seperating a list into 2 by true or false
    if possibly_prime in primes:
        true_quotient.append(possibly_prime)
    else:
        false_quotient.append(possibly_prime)

 # [...]

much better now. Now we have the chance to extract functions. From

primes = [2,3]
exponent = 3
limit = int(10**exponent)
for number in range(len(primes)+2, limit, 1):
    possibly_prime = Primer(number, primes)     # feeds primer with numbers and past primes...
    if type(possibly_prime) == int:
        primes.append(possibly_prime)           # when prime is found it gets put into cycles

we make

def get_primes(limit):
    primes = [2,3]
    for number in range(len(primes)+2, limit, 1):
        possibly_prime = Primer(number, primes)     # feeds primer with numbers and past primes...
        if type(possibly_prime) == int:
            primes.append(possibly_prime)           # when prime is found it gets put into cycles
    return primes

exponent = 3
limit = int(10**exponent)
primes = get_primes(limit)

If you do that with all your first level loops (and move the function definitions before all the remaining (unintended) code, this remaing code at the bottom be much more readable. At least if you chose good function names. You also immediately see what parameters a function needs. At this point you can improve naming and refactor your functions.


Now we also do this for the block

window = [0, limit]
primes_for_triangulation = []
use_primes_up_to = e**(exponent)
primes_in_window = []
for prime in primes:#creating a list from part of list
    if prime < use_primes_up_to:
        primes_for_triangulation.append(prime)
    else:
        pass
    if prime >= window[0] and prime < window[1]:#remenant from when window was different than zero to limit
        primes_in_window.append(prime)
    else:
        pass

We get

def get_primes_for_triangulation(primes, use_primes_up_to):
    primes_for_triangulation = []
    for prime in primes:                                # creating a list from part of list
        if prime < use_primes_up_to:
            primes_for_triangulation.append(prime)
    return primes_for_triangulation

def get_primes_in_window(primes, window):
    primes_in_window = []
    for prime in primes:                                # creating a list from part of list
        if prime >= window[0] and prime < window[1]:    # remenant from when window was different than zero to limit
            primes_in_window.append(prime)
    return primes_in_window

use_primes_up_to = e**(exponent)
primes_for_triangulation = get_primes_for_triangulation(primes, use_primes_up_to)

window = [0, limit]
primes_in_window = get_primes_in_window(primes, window)

As I said before we now have a chance to take a close look at the extracted functions. We notice that they are trivial and can be replaced by list comprehension.

def get_primes_for_triangulation(primes, use_primes_up_to):
    return [p for p in primes if p < use_primes_up_to]

def get_primes_in_window(primes, window):
    # remenant from when window was different than zero to limit
    return [p for p in primes if p >= window[0] and p < window[1]]

You may now inline the comprehensions directly in your main body as the comprehensions are trivial. Note: Sometimes it is desirable to keep the functions as the main code may be more readable and one has the chance to add docstrings to explain the algorithms inside the functions.


Now let's have a look at Primer. We start with your function

def Primer(number, numbers):                    # Generates primes by modulus for every prime in numbers
    iteration = 0
    while number % numbers[iteration] != 0:     # as long as number modulus prime givesw remainder it keeps dividing
        iteration +=1
        if iteration == len(numbers):           # Every former prime in list of primes hass been divided with remainder
            return number
        else:
            pass

We immediately see there is a superfluous else branch that we remove

def Primer(number, numbers):                    # Generates primes by modulus for every prime in numbers
    iteration = 0
    while number % numbers[iteration] != 0:     # as long as number modulus prime givesw remainder it keeps dividing
        iteration +=1
        if iteration == len(numbers):           # Every former prime in list of primes hass been divided with remainder
            return number

If you iterate over a range or any other iterable (list, ...) you use a for loop. All the error prone fiddling with indices (off by one, ...) are gone. We rewrite to

def Primer(number, numbers):                    # Generates primes by modulus for every prime in numbers
    for n in numbers:
        if number % n == 0:
            return
    return number

Now we see a function that returns a single number that was passed as a parameter or it returns None. This smells like a boolean test. So the function

  • shall return a boolean
  • shall have a name starting with is_ allowing it to be used in a readable test

The caller now gets a bool which is no problem as he knows the number anyway. We get

def is_divisible(number, numbers):                    # Generates primes by modulus for every prime in numbers
    for n in numbers:
        if number % n == 0:
            return True
    return False

which we shorten with the help of any to

def is_divisible(number, numbers):                    # Generates primes by modulus for every prime in numbers
    return any(number % n == 0 for n in numbers)

which we now could easily inline as well.

Now to the caller. Your function

def get_primes(limit):
    primes = [2,3]
    for number in range(len(primes)+2, limit, 1):
        possibly_prime = Primer(number, primes)     # feeds primer with numbers and past primes...
        if type(possibly_prime) == int:
            primes.append(possibly_prime)           # when prime is found it gets put into cycles
    return primes

has to use the new test like

def get_primes(limit):
    primes = [2,3]
    for number in range(len(primes)+2, limit, 1):
        if not is_divisible(number, primes):
            primes.append(number)           # when prime is found it gets put into cycles
    return primes

we got rid of this type test and possibly_prime and have a nice readable function. No we do the inlining where we invert the logic to better fit the caller

def get_primes(limit):
    primes = [2,3]
    for number in range(len(primes)+2, limit, 1):
        if all(number % n != 0 for n in primes):
            primes.append(number)           # when prime is found it gets put into cycles
    return primes

Nice and readable. So readable, that we immediately start doubting the algorithm. Especially the range is horrible. Why start at len(primes)+2? Why step 1? Why test for the whole list of primes?

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why are u so angry? \$\endgroup\$ – Progrmming is fun Jan 22 at 15:13
  • \$\begingroup\$ As for the semi constructive inprovements (semi because it went over my head, obviuosly) I will look into your feedback "Scan" "Functions in functions" "Overwrite variables and or lists" "Refactor functions meaning " "Ad docstrings in between functions in main" "Why is else pass superflous (whats else pass for)" "What is indices" ? \$\endgroup\$ – Progrmming is fun Jan 22 at 15:17
  • \$\begingroup\$ Is theire a single question about how to check for primes? \$\endgroup\$ – Progrmming is fun Jan 22 at 15:20
  • \$\begingroup\$ A tag about how to check for primes? Are u even interested as to why? \$\endgroup\$ – Progrmming is fun Jan 22 at 15:20
  • 1
    \$\begingroup\$ @Progrmmingisfun I fail to see anything that is aggressive or hostile in the above post. To me the answer just reads as unemotional ways to improve your code. Given the large amount of comments you've left here, were you in a conversation with the answerer? \$\endgroup\$ – Peilonrayz Jan 26 at 0:13

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.