current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 9 down vote favorite

I have 5 nested for loops below, to add rows of data from multiple files to one of two lists. Is there a more pythonic way of doing this? I've come across an iterator-generator method named iteritems() -- could this be used to make this code more pythonic? 

# 5 nested loops

for root,dirs,files in os.walk(src):
   files = [ _ for _ in files if _.endswith('.xlsx') ]
   for file in files:
     wb = xlrd.open_workbook(os.path.join(root,file))
     worksheets = wb.sheet_names()
     for worksheet_name in worksheets:
         if worksheet_name.rfind('7600') != -1 :
            sheet = wb.sheet_by_name(worksheet_name)
            keys = [sheet.cell(3, col_index).value for col_index in xrange(sheet.ncols)]
            for row_index in xrange(4, sheet.nrows):
               d = {keys[col_index]: sheet.cell(row_index, col_index).value
                  for col_index in xrange(sheet.ncols)}
               if file.rfind('oam') != -1 :
                  list_7600EoX.append(d)
               else:
                   list_7600EoX_OAM.append(d)
share|improve this question

migrated from stackoverflow.com May 26 at 1:13

This question came from our site for professional and enthusiast programmers.
7  
It may be an indication that you should write your own class and wrap things inside it. Or separate responsibilities to different functions. –  Maroun Maroun May 25 at 12:38
    
When confronted with situations like this, I frequently break inner loops out into separate functions. But I feel that we might need to know more to suggest a specific solution. What's your end goal? –  senderle May 25 at 12:46
1  
It's a bit gnarly. You can remove one level of nesting by using glob instead of the outer two loops. And you can avoid the extra indentation of that first conditional by inverting the logic: if worksheet_name.rfind('7600') == -1: continue –  wim May 25 at 13:24
    
@wim Can glob really be told to go through all subdirectories? How? –  Stefan Pochmann May 25 at 13:48
    
No, sorry, glob can't do that. Seems I was talking rubbish :) –  wim May 25 at 14:10

3 Answers 3

up vote 12 down vote accepted

Your problem is not with 5 (or more) loops. Your problem is that you mixing code of different nature: walking over filesystem and name matching, and processing of files into single chunk of code.

Separate it into different functions calling each other:

def process_worksheet(wb, worksheet_name):
    sheet = wb.sheet_by_name(worksheet_name)
    # ...

def process_xslx(path):
    wb = xlrd.open_workbook(path)
    worksheets = wb.sheet_names()
    for worksheet_name in worksheets:
        if worksheet_name.rfind('7600') != -1 :
            process_worksheet(wb, worksheet_name)

for root,dirs,files in os.walk(src):
    files = [ _ for _ in files if _.endswith('.xlsx') ]
    for file in files:
        process_xslx(os.path.join(root, file))

Another option is to use generators to hide some details on how iteration is performed. For example, instead of walking over filesystem, let generator yield workbooks:

def walk_xlsx(src):
    for root,dirs,files in os.walk(src):
        files = [ _ for _ in files if _.endswith('.xlsx') ]
        for file in files:
            wb = xlrd.open_workbook(os.path.join(root, file))
            yield wb

for wb in walk_xlsx(src):
    # filter() is also a generator which yields only 
    # worksheet names that have '7600' in their names 
    worksheets = filter(lambda wn: '7600' in wn, wb.sheet_names())
    for worksheet_name in worksheets:
        # ...
share|improve this answer
    
That's definitely a lot more readable than the code in the OP. :) –  PM 2Ring May 25 at 14:05
    
This does look a lot better. But I would say that five loops is a likely problem under any circumstances -- even if the loops are all doing things of the same nature. Loops alone add cognitive load. Linus says no more than three indentations total! That's two loops per function (because functions are already indented once). There's no hard limit but a firm maximum of four is a good rule of thumb. –  senderle May 25 at 14:24
1  
@myaut Thanks! This is great and exactly the feedback I was hoping for. Using functions makes a lot of sense to simply my code and I like the option using generators to further hide some details. Cool stuff! I'm also read the response from Kyle and I need to determine if it makes sense to combine his recommendation to use Pandas with yours -- I don't know if it simplifies things much more. –  Jim Durkin May 25 at 15:31
    
This alone is pretty simple and gets the job done. I'd only refactor it to using pandas if you end up doing a bunch more columnar and row operations. Pandas gives great power but takes some wrapping your head around. –  Kyle Kelley May 25 at 18:31
    
I've been trying to use the generator approach, however, i can't seem to determine how to pass the file name from the generator so that I can use my logic "if file.rfind('oam') != -1" under "for worksheet_name in worksheets:". –  Jim Durkin May 25 at 23:41
up vote 6 down vote

As senderle mentions in the comments, one way to flatten things like this is to put the body of each for loop into a function. This makes the code slightly slower, due to the overhead of calling functions, but it does make it easier to read, not just because of the reduced indentation, but also because it's more modular, so it becomes more obvious what variables are being affected by each section of the code.

In some cases it's possible to flatten nested loops by using the itertools.product method, but that's not applicable in your case.

As the docs say:

Equivalent to nested for-loops in a generator expression. For example, product(A, B) returns the same as ((x,y) for x in A for y in B).

You need to pass all of the iterables to product when you call it, but in your program those iterables only become known as you descend into the loops.

But anyway, for future reference, here's a short Python 2 demo:

from itertools import product

r1 = (0, 1, 2)
r2 = (0, 10, 20)
r3 = (0, 100, 200)
for v3, v2, v1 in product(r3, r2, r1):
    print '%3d + %2d + %1d = %3d' % (v3, v2, v1, v3 + v2 + v1)

output

  0 +  0 + 0 =   0
  0 +  0 + 1 =   1
  0 +  0 + 2 =   2
  0 + 10 + 0 =  10
  0 + 10 + 1 =  11
  0 + 10 + 2 =  12
  0 + 20 + 0 =  20
  0 + 20 + 1 =  21
  0 + 20 + 2 =  22
100 +  0 + 0 = 100
100 +  0 + 1 = 101
100 +  0 + 2 = 102
100 + 10 + 0 = 110
100 + 10 + 1 = 111
100 + 10 + 2 = 112
100 + 20 + 0 = 120
100 + 20 + 1 = 121
100 + 20 + 2 = 122
200 +  0 + 0 = 200
200 +  0 + 1 = 201
200 +  0 + 2 = 202
200 + 10 + 0 = 210
200 + 10 + 1 = 211
200 + 10 + 2 = 212
200 + 20 + 0 = 220
200 + 20 + 1 = 221
200 + 20 + 2 = 222
share|improve this answer
up vote 3 down vote

The smallest components (which could be the very last, very spreadsheet specific operations) should certainly be broken up into a function so that:

  • It's easier to read
  • Easier to unit test

However, looking over this code and the type of operations you're doing, you should try out Pandas using its read_excel.

Additionally, some of the things you want to be done you can do as separate loops or comprehensions to filter out things you don't want.

Example with the two combined:

for root,dirs,files in os.walk(src):
  filenames = [ os.path.join(root,_) for _ in files if _.endswith('.xlsx') ]
  for filename in filenames:

    # This returns a dict of DataFrames or a single DataFrame when only one sheet
    # TODO: Detect if single sheet vs. multiple sheet. This assumes multiple sheets
    dfs = pandas.io.excel.read_excel(filename) # If sheetname known, can be passed

    dfs = {name: df for (name, df) in dfs if '7600' in name}

    df = dfs[name]

    # DataFrame operations for the rest...

Beyond that, I also noticed you used rfind just to check for the existence of a string. You might as well use in: '7600' in name.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.