3
\$\begingroup\$

I saw that there was a previous question asking to review an answer to the Project Euler question #8. My answer didn't look anything like the answer in that question and as a beginner, I was wondering if someone would take a look at my answer and let me know if there is anything I could improve upon.

#Project Euler #8

'''
The four adjacent digits in the 100 digit number that have the greatest
product are 9x9x8x9 = 5832.

Find the 13 adjacent digits in the 1000 digit number that have the 
greatest product
'''


n = '\
73167176531330624919225119674426574742355349194934\
96983520312774506326239578318016984801869478851843\
85861560789112949495459501737958331952853208805511\
12540698747158523863050715693290963295227443043557\
66896648950445244523161731856403098711121722383113\
62229893423380308135336276614282806444486645238749\
30358907296290491560440772390713810515859307960866\
70172427121883998797908792274921901699720888093776\
65727333001053367881220235421809751254540594752243\
52584907711670556013604839586446706324415722155397\
53697817977846174064955149290862569321978468622482\
83972241375657056057490261407972968652414535100474\
82166370484403199890008895243450658541227588666881\
16427171479924442928230863465674813919123162824586\
17866458359124566529476545682848912883142607690042\
24219022671055626321111109370544217506941658960408\
07198403850962455444362981230987879927244284909188\
84580156166097919133875499200524063689912560717606\
05886116467109405077541002256983155200055935729725\
71636269561882670428252483600823257530420752963450'

#Import Modules
import operator
from operator import mul

#Put all of the 
def greatest_product():
    global list
    list = []
    for i in n:
        list.append(int(i))
    return list

greatest_product()

def lists():
    global seq_13
    seq_13 = []

    count = 0
    for i in list:
        x = list[count:count+13]
        if len(x) == 13:
            seq_13.append(x)
            count += 1
    return seq_13
print lists()

num_list_fin = []
for num_item in seq_13:
    num_item_u = [reduce(lambda x,y: x*y, num_item)]
    num_item_u.append(num_item)
    num_list_fin.append(num_item_u)

num_list_fin.sort(key=lambda x: int(x[0]))
print num_list_fin[len(num_list_fin)-1]
\$\endgroup\$
3
\$\begingroup\$

Rename your variables and functions to be more specific and do not keep unneeded array data, like so:

def createDigits():
    global digits
    digits = []
    for i in n:
        digits.append(int(i))
    return digits

def calculateProduct(startPosition):
   global digits
   global windowSize
   factors = digits[startPosition: startPosition+windowSize]
   product = 1
   for digit in digits:
       product = product * digit
   return product

createDigits()
maxProduct = 1
windowSize = 13
for i in xrange(0, len(digits - windowSize))
   product = calculateProduct(i)
   if product > maxProduct:
      maxProduct = product

EDIT: Also, might as well not scan products at the end when the window starts to shrink, since they are guaranteed to be less than the last complete window.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

General issues

  • Stray imports: You never use operator and mul (though mul could have been useful).
  • Poor function names: The greatest_product() function is misnamed: its task is to convert a string into a list of digits. The lists() function could also be named more descriptively — I suggest consecutive_groups().
  • Use of global variables: Function should take input parameters and return its results. Instead, you implicitly take the inputs from the environment and pass the results back via global variables. (You wrote the code with returns, but in practice you are relying on global variables instead.)

Reorganizing the code based on just the points above…

def digits(numeric_string):
    list = []
    for i in n:
        list.append(int(i))
    return list

def consecutive_groups(chunk_size, list):
    sequences = []

    count = 0
    for i in list:
        x = list[count:count+chunk_size]
        if len(x) == 13:
            sequences.append(x)
            count += 1
    return sequences

n = '…'
seq_13 = consecutive_groups(13, digits(n))
…

It's clearer how seq_13 is derived when the code uses functions as intended rather than relying on side effects.

List handling

You build a lot of lists, starting with an empty list [] and .append()ing entries to them. In Python, that can be better expressed as a list comprehension.

def digits(numeric_string):
    return [int(i) for i in n]

def consecutive_groups(chunk_size, list):
    return [list[count:count+chunk_size] for count in xrange(len(list) - chunk_size)]

To find the maximum product, you don't need to sort; just use the max() function. Conveniently, max() lets you specify a criterion via a function.

def product(list):
    return reduce(lambda x, y: x * y, list)

n = '…'
seq_13 = consecutive_groups(13, digits(n))
print max(seq_13, key=product)
| improve this answer | |
\$\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.