Multiplying over a matrix

The below code is a solution I put together for the Project Euler #11 question which asked to find the largest product of 4 numbers of any direction in a matrix (horiz, vertic, and diag all acceptable.)

-
Coding methodology: To me this is the most important. Is the algorithm I used feasible or mostly correct, if not how do I correct it and why won't it work or scale? I realize in some cases there are always going to be optimal solutions that will be over my head mathematically, but generally you can yield a solution that is within a reasonable range of optimal that is passable without knowing a ton of number theory in my experience. Where possible, I hope to learn algorithms along the way (such as the Sieve of Eratosthenes for prime ranges).

-
Coding execution: Given some pseudocode that we determine algorithmically will work, is the code written executed without redundancies and minimizing runtime. As you can see below, my coding is terrible and I'm sure violates many of the principles here.

-
Being more pythonic: Is something I'm not doing pythonic or can it be done simpler with some sort of library I don't know about. I believe this is mostly semantic though in some cases can have large effects on performance (for instance using map() instead of a for loop to apply int()).

After looking through another submitted solution on Code Review, I saw that:

lines = [s for s in b.splitlines()]

bb = []
i = 0
for line in lines:
    bb.append(map(int,lines[i].split()))
    i += 1

can simplify to:

bb = [map(int, row.split()) for row in open('number.txt').read().splitlines()]

```
b = """08 02 22 97 38 15 00 40 00 75 04 05 07 78 52 12 50 77 91 08
49 49 99 40 17 81 18 57 60 87 17 40 98 43 69 48 04 56 62 00
81 49 31 73 55 79 14 29 93 71 40 67 53 88 30 03 49 13 36 65
52 70 95 23 04 60 11 42 69 24 68 56 01 32 56 71 37 02 36 91
22 31 16 71 51 67 63 89 41 92 36 54 22 40 40 28 66 33 13 80
24 47 32 60 99 03 45 02 44 75 33 53 78 36 84 20 35 17 12 50
32

A few suggestions for improving your code:

-
You can tidy up this line:

lines = [s for s in b.splitlines()]

to simply

lines = b.splitlines()

as your list comprehension doesn’t affect the elements, and .splitlines() already returns a list in Python 2.7.

Indeed, you could go one step further and cut out the lines variable, and iterate over this directly:

for line in b.splitlines():

-
Let’s break down this loop:

bb = []
i = 0
for line in lines:
    bb.append(map(int,lines[i].split()))
    i += 1

The whole point of using a for foo in bar-style loop is that it saves you doing the index lookups; you can access the variable directly. So you can do away with the i variable, and rewrite it like this:

bb = []
for line in lines:
    bb.append(map(int,line.split()))

Generally I think you should prefer list comprehensions to using map(), because it makes for more readable code. Brevity is good; readability is better. Don’t squash too much onto one line. So I’d expand it slightly like this:

bb = []
for line in lines:
    bb.append([int(num) for num in line.split()])

We could reduce this to a one-line list comprehension:

bb = [[int(num) for num in line.split()] for line in lines]

I think that’s right on the cusp of doing too much in a single line. More than two loops within the same list comprehension is definitely too complicated; this is right on the boundary.

Personally I’d err towards the first construction, but I can also see the case for the second.

(Re-reading the question, I see you mention the performance benefits of map() vs a for loop. Unless you’re working with very large data sets, the difference is probably negligible. Try not to fall into the trap of premature optimisation.)

-
In this loop:

row_ct = 0
for row in bb:
    column_ct = 0
    for column in row:
        # do stuff
        column_ct += 1
    row_ct += 1

you should look up the enumerate() function, which can save you from manually managing the indices. It looks like this:

for row_idx, row in enumerate(bb):
    for col_idx, col in enumerate(row):
        # do stuff

-
Try to use more descriptive variable names – bb and n don’t tell me much about what these variables represent. Using names that describe your variables will make your code easier to read, review and debug later.

-
You’re storing every possible product, even though you’re only interested in the largest. A quick back-of-the-napkin calculation shows that count (another poor variable name, btw) will end up with over 1000 items.

Most of them can be thrown away before they're added to count, because they're smaller than an existing product.

You should rewrite your program to only store the largest product seen so far – that’s a much more scalable approach.

StackExchange Code Review Q#105060, answer score: 4