1
\$\begingroup\$
import math
import time
import sys
import random
import os

def isPrime(number):
    if number % 2 != 0:
        for i in range (3,math.floor(number**0.5)+1,2):
            if number % i == 0:
                return False
    else:
        if number != 2:
            return False
    return True

def makeListInts(lst):
    for item in range(len(lst)):
        try:
            lst[item] = int(lst[item])
        except:
            try:
                lst[item] = float(lst[item])
            except:
                pass
    return lst

def getData():
    with open('CurrentProgress.txt') as data:
        return makeListInts([line[:-1].split(': ')[1] for line in data.readlines()])

def setData(data, ID):
    titles = [
    'Start: ' + str(data[1] + 1),
    'End: ' + str(data[1] + data[2]),
    'Step: ' + str(data[2]),
    'Time: ' + str(time.time()),
    'IDsRegistered: ' + ID]
    with open('CurrentProgress.txt','w') as data:
        for item in titles:
            data.write(str(item)+'\n')

def checkFileProgress(data):
    files = os.listdir('Calculated Primes/')                    #List all primes files
    filesStripped = [int(i.split('-')[1][:-4]) for i in files]  #Get the end numbers of each file
    filesStripped.sort()                                        # and sort
    diffs = [filesStripped[i+1] - filesStripped[i] for i in range(len(filesStripped)-1)]
    lastFileEnd = filesStripped[-1]
    #print (diffs, sum(diffs) / len(diffs))
    if max(diffs) != sum(diffs) / len(diffs):  #If there is one that should not be there (max != avg)
        indx = diffs.index(max(diffs))
        print (indx)
        data = [filesStripped[indx-1] + 1, filesStripped[indx-1] + data[2], data[2], data[3],data[4]] #Go back to there
        setData(data,data[-1])
    elif lastFileEnd + 1 != data[0]:  # Otherwise, if the last file is before the recoreded end , go back there
        data = [lastFileEnd - data[2] + 1, lastFileEnd, data[2], data[3], data[4]]
        setData(data, data[-1])

def setMyID(currentIDs):
    possibleIDs = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L']
    if currentIDs == ' ':
        return possibleIDs[0]
    else:
        try:
            return possibleIDs[possibleIDs.index(currentIDs[-1]) + 1]
        except IndexError:
            return False

def removeMyID(data,ID):
    ID = data[-1].replace(ID,'')
    setData(data, ID)
    print ()


def writePrimes(listOfPrimes, data):
    direc = 'Calculated Primes/'
    with open(direc + 'Primes' + str(data[0]) + '-' + str(data[1]) + '.txt','w') as primes:
        for prime in listOfPrimes:
            primes.write(str(prime) + '\n')

def checkTime(dataTime):
    if dataTime + 1 > time.time():
        sleepTime = round(random.random()+1,3) #01179107383 James
        print ('Sleeping for ' + str(sleepTime) + ' to avoid collisions')
        time.sleep(sleepTime);
        return False
    else:
        return True

def makeBar(barWidth,data):
    print ('Calculating primes between ' + str(data[0]) + ' and ' + str(data[1]) + ':')
    sys.stdout.write("[%s]" % (" " * barWidth))
    sys.stdout.flush()
    sys.stdout.write("\b" * (barWidth+1)) # return to start of line, after '['

def updateBar():
    sys.stdout.write("==")
    sys.stdout.flush()

def closeBar():
    sys.stdout.write("\n")

def main(ID):
    data = getData()
    if checkTime(data[3]):
        barWidth = 40
        if ID in data[-1]:
            ID = data[-1]
        else:
            ID = data[-1] + ID
        setData(data,ID)
        makeBar(barWidth, data)
        listOfPrimes = []
        for number in range(data[0],data[1]):
            result = isPrime(number)
            if (number - 1) % round(data[2] / (barWidth / 2)) == 0:
                updateBar()
            if result:
                listOfPrimes += [number]
        writePrimes(listOfPrimes, data)
        closeBar()
    else:
        main(ID)

def setup():
    data = getData()
    if data[-1] == ' ':
        checkFileProgress(data)
    if checkTime(data[3]):
        ID = setMyID(data[-1])
        print (ID)
        if ID:
            try:
                while True:
                    main(ID)
            except KeyboardInterrupt:
                data = getData()
                removeMyID(data,ID)
                quit('Saving and Exiting')
        else:
            quit('Already 12 programs running')
    else:
        setup()

setup()

Here is the file it accesses (CurrentProgress.txt):

Start: 381000001
End: 382000000
Step: 1000000
Time: 1468445801.017807
IDsRegistered:  

The last line of the file has 2 trailing spaces.

It has an ID function, allowing any ID from A-L. A script is prevented from running if there are already 12. Each script waits for a random time to prevent collisions with others (which is unlikley anyway). When the script is terminated, the ID is removed. Because the file is updated before calculations are finished, when the first script is run it sets the file back to the next block (by checking the file). This only happens for the first script (i.e. that which takes ID A). Equally, it checks if a block has been skipped (in case the scripts are terminated in the wrong order) and jumps back to there.

I realise this code is quite long (150 lines) so if you feel like picking a function to review it would still help me learn!

\$\endgroup\$
1
  • \$\begingroup\$ I expect that it would be sensible to load the file differently, to strip the data out. It's done this way to make the file readable too. \$\endgroup\$
    – Tim
    Commented Jul 13, 2016 at 21:50

1 Answer 1

1
\$\begingroup\$

I'll start with a few items and one function.

  • I suggest you use lower case for all variables and functions
  • I'd also suggest you use lowercase with underscores for functions, it does make it easier to read in most cases. You can read the python style guide here for more info and suggestions to match other python code.
  • making your variable names descriptive would go a long way in making your code a bit easier to follow, You use the word data all over the code, but that is a very generic word. Perhaps instead of writePrimes(listOfPrimes, data): you can try write_primes(prime_list, filename_list)
  • Unfortunately the code is very hard to follow as it is right now, I think giving more descriptive names for variables and methods will give us a clearer picture of what you are trying to do here.
  • One minor note, if you are going to be making large lists, I would recommend you use xrange as opposed to range, as xrange is lazily evaluated and range evaluates the sequence at runtime.
  • One final note, for calculating primes typically memoization is the easiest way calculate a list of primes quickly, (Although there are several other ways to do this as well). Just in case you weren't aware wanted to let you know.

For the function I've chosen setData

def setData(data, ID):
    titles = [
    'Start: ' + str(data[1] + 1),
    'End: ' + str(data[1] + data[2]),
    'Step: ' + str(data[2]),
    'Time: ' + str(time.time()),
    'IDsRegistered: ' + ID]
    with open('CurrentProgress.txt','w') as data:
        for item in titles:
            data.write(str(item)+'\n')

Should be split up to two:

def get_titles(data, id):
    return [
      'Start: {0}'.format(data[1] + 1),
      'End: {0}'.format(data[1] + data[2]),
      'Step: {0}'.format(data[2]),
      'Time: {0}'.format(time.time()),
      'IDsRegistered: ' + id]

def write_file(data, id):
"""Writes a file full of prime numbers

   data is a list consisting of a start and a step value,
   id is the name of the file registered"""
    with open('CurrentProgress.txt','w') as out_file:
        for item in get_titles(data, id):
            out_file.write(str(item)+'\n')

In summary, I think if you use more descriptive names in your code throughout, and a bit of commenting if the parameters or method name is unclear, then it will be much easier to see where functions can be modularized and to give you a better code review.

EDIT: One more function

def checkFileProgress(data):
    files = os.listdir('Calculated Primes/')                    #List all primes files
    filesStripped = [int(i.split('-')[1][:-4]) for i in files]  #Get the end numbers of each file
    filesStripped.sort()                                        # and sort
    diffs = [filesStripped[i+1] - filesStripped[i] for i in range(len(filesStripped)-1)]
    lastFileEnd = filesStripped[-1]
    #print (diffs, sum(diffs) / len(diffs))
    if max(diffs) != sum(diffs) / len(diffs):  #If there is one that should not be there (max != avg)
        indx = diffs.index(max(diffs))
        print (indx)
        data = [filesStripped[indx-1] + 1, filesStripped[indx-1] + data[2], data[2], data[3],data[4]] #Go back to there
        setData(data,data[-1])
    elif lastFileEnd + 1 != data[0]:  # Otherwise, if the last file is before the recoreded end , go back there
        data = [lastFileEnd - data[2] + 1, lastFileEnd, data[2], data[3], data[4]]
        setData(data, data[-1])

following the same guidelines as above could be written like this:

def get_last_number(file):
    return int(file.split('-')[1][:-4])

def get_sorted_file_numbers(files):
    files_stripped = [get_last_number(i) for file in files]  # get the last number of each file
    files_stripped.sort()                                       # and sort
    return files_stripped

def get_diffs(file_numbers)
    return [file_numbers[i+1] - file_numbers[i] for i in range(len(file_numbers)-1)]

def get_file_data(diffs, file_numbers):
    last_file_number = file_numbers[-1]
    if max(diffs) != sum(diffs) / len(diffs):  #If there is one that should not be there (max != avg)
        indx = diffs.index(max(diffs))
        print (indx)
        data = [file_numbers[indx-1] + 1, 
                file_numbers[indx-1] + file_numbers[2],
                file_numbers[2], 
                file_numbers[3],
                file_numbers[4]] #Go back to there
        return data
    elif last_file_number = file_numbers[-1] + 1 != file_numbers[0]:  # Otherwise, if the last file is before the recoreded end , go back there
        data = [last_file_number - file_numbers[2] + 1,
                last_file_number,
                file_numbers[2],
                file_numbers[3],
                file_numbers[4]]
        return data

def checkFileProgress(data):
    files = os.listdir('Calculated Primes/')                    #List all primes files
    file_numbers = get_sorted_file_numbers(files)
    diffs = get_diffs(file_numbers)
    prime_data = get_file_data(diffs, file_numbers)
    write_file(prime_data, prime_data[-1])

Making your code modular allows it to be testable piece by piece. Then you can just test get_file_data with a small list of diffs and file_numbers. to make sure that the data you are getting is correct.

\$\endgroup\$
3
  • \$\begingroup\$ A couple of points and questions. 1. The style guide seems to have no preference over these. 2. Variable names, yes, will do. 3. The runs in python 3.5, so xrange is not usable. 4. I'm not sure memoization is realistic here - I've calculated all the primes below 700,000,000 so far this evening - I don't know if storing those in memory is realistic - currently it dumps to file every 1,000,000? 5. Finally, why would you suggest splitting it up into the two functions? \$\endgroup\$
    – Tim
    Commented Jul 13, 2016 at 23:03
  • \$\begingroup\$ Sure thing. 1. You are correct style guide tends to not really care, but suggests to use lowercase_words syntax as here: python.org/dev/peps/pep-0008/#function-names \$\endgroup\$
    – canoycano
    Commented Jul 14, 2016 at 0:32
  • 1
    \$\begingroup\$ 3. Gotcha, in python3.5 xrange replaces range. 4. AHH, that makes sense. I was pretty unclear about what your code did just by reading it, so the code dumps to file every 1mil primes found. Gotcha. 5. You'll always want to make each function as modular and reusable as possible, performing one clear concise job. Here you are having a function to do two things, format your output and then write that output to a file. \$\endgroup\$
    – canoycano
    Commented Jul 14, 2016 at 0:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.