# Indenting a function to recursively find filenames

I wrote this function that recursively traverses a given directory resolves the relative file-names to absolute file-names and yields the resolved file-name if it matches a given pattern. You can specify a callback function as well.

The first issue I have with this function is that it is a bit hard to comprehend, I think this problem can be reduced with proper indentation of the generator expressions.

The second thing I would like help with is, How do i reduce the repeated block of code?

import fnmatch
import os
import sys

def find(root, pattern, *callback):
if not callback:
for _, _, files in  ((root, _, (os.path.join(root, filename)
for filename in files if fnmatch.fnmatch(filename, pattern)))
for (root, _, files) in os.walk(root)):
for filename in files:
yield filename

callback = callback[0]
for _, _, files in  ((root, _, (os.path.join(root, filename)
for filename in files if fnmatch.fnmatch(filename, pattern)))
for (root, _, files) in os.walk(root)):
for filename in files:
yield callback(filename)

def cb(filename):
print filename

for filename in find('/home/dwilson', '*.py', cb):
pass


## migrated from stackoverflow.comFeb 15 '15 at 4:34

This question came from our site for professional and enthusiast programmers.

• You seem to be very concerned about performance. Keep in mind, however, that most of the work involves the OS and filesystem I/O. You should therefore avoid mangling your code in search of optimizations that have negligible performance benefit. – 200_success Feb 15 '15 at 8:27

A good way would be to eliminate duplicate using a default callback that does nothing:

def walker(root, pattern):
for _, _, files in  ((root, _, (os.path.join(root, filename)
for filename in files if fnmatch.fnmatch(filename, pattern)))
for (root, _, files) in os.walk(root)):
for filename in files:
yield filename

def find(root, pattern, callback=lambda v:v):
for filename in walker(root, pattern):
yield callback(filename)


Afterwards, we simplify the walker to

def walker(root, pattern):
for (root, _, files) in os.walk(root):
for filename in files:
if fnmatch.fnmatch(filename, pattern):
yield os.path.join(root, filename)


and have a much more readable thing than before.

• This approach is to slow, generator expressions are far faster then for loops, Also they are more pythonic. – Ricky Wilson Feb 13 '15 at 17:12
• I just made timeit measurements and found that the 2nd approach is indeed slightly faster - but only a few %. And readable code is definitely more pythonic than unreadable one... – glglgl Feb 13 '15 at 17:21
• If space is not a constraint and speed is paramount, list comprehensions are usually faster than generators because generators generate a bit of overhead. – gboffi Feb 13 '15 at 18:14
• @myself For large amount of data generators can be not only more efficient but also faster. – gboffi Feb 13 '15 at 18:23
• @RickyWilson, I strongly doubt that pylint may recognize cluttered code with over-complicated logic..... I can see one redundancy right now - asterick operator before callback. I would say that more Pythonic approach would be default None value - though I prefer my default lambda twist – volcano Feb 13 '15 at 22:49

I think I need my own space to explain "my approach" with default lambda - thanks to glglgl for hospitality :) .

It saves you from having to check condition in the function. And the overhead? Not that much (here comes dis, which I - alas! - nearly never use)

dis.dis(lambda v: v)
3 RETURN_VALUE


But how bad is it - compared to other approaches? See below

>>> def test1(param, callback=lambda v:v):
return callback(param)

>>> def test2(param, *callback):
if callback:
return callback[0](param)
else:
return param

>>> def test3(param, callback=None):
if callback is not None:
return callback(param)
else:
return param

>>> dis.dis(test1)
6 CALL_FUNCTION            1
9 RETURN_VALUE
>>> dis.dis(test2)
3 POP_JUMP_IF_FALSE       20

12 BINARY_SUBSCR
16 CALL_FUNCTION            1
19 RETURN_VALUE

5     >>   20 LOAD_FAST                0 (param)
23 RETURN_VALUE
27 RETURN_VALUE
>>> dis.dis(test3)
6 COMPARE_OP               9 (is not)
9 POP_JUMP_IF_FALSE       22

18 CALL_FUNCTION            1
21 RETURN_VALUE

5     >>   22 LOAD_FAST                0 (param)
25 RETURN_VALUE
29 RETURN_VALUE


Surprise, surprise, None loses by 2 bytecodes (did not see it coming), still - it is cleaner that *callback, which in semantically misleading, since it implies possibility of more than one value. And default callback overhead is less than 25% of any

As for run-time impact - try timing runs with different implementations, and see for yourself

Well.. I got rid of the duplicated block of code but the list comprehensions are still hard to follow, And i wrote them...

I don't like how for each iteration of the loop the function has to check to see if a callback was given.

import fnmatch
import os
import sys

def find(root, pattern, *callback):

for _, _, files in  ((root, _, (os.path.join(root, filename)
for filename in files if fnmatch.fnmatch(filename, pattern)))
for (root, _, files) in os.walk(root)):
for filename in files:
if not callback:
yield filename
else:
yield callback[0](filename)

def print_filename(filename):
print filename

for filename in find('/', '*.c', print_filename):
pass

• My approach would be checking once, like callback = callback[0] if callback else lambda v: v. Even (IMHO) better approach - do it in function signature def find(root, pattern, callback=lambda v:v): – volcano Feb 13 '15 at 21:48
• @volcano Your approach would cause a lambda function to be called for every iteration for no reason, Or maybe I'm confused. The reason I chose to use optional args aka *args is that i wanted the callback function to be optional. Could you elaborate more on your approach? PS. Thanks from the input. – Ricky Wilson Feb 14 '15 at 1:53

I took the liberty of using shortened names due to the constraints of interactivity but I hope that you can decide if this suggestion is good enough for your use case.

Here follows a (slightly edited) ipython interactive session.

In [38]: from os.path import join as j
In [39]: from os import walk as w
In [40]: from fnmatch import fnmatch as fm
In [41]: def x(r,p,*c):
g = (j(r,fn) for r, dl, fl in w(r) for fn in fl if fm(fn,p))
return (c[0](fn) for fn in g) if c else g
....:
In [42]: x('bin','*py*')
Out[42]: <generator object <genexpr> at 0x7fdf7411cc30>
In [43]: list(x('bin','*py*'))
Out[43]:
['bin/pythontex_engines.py',
...,
'bin/pythontex2.py']
In [44]: x('bin','*py*',lambda s:s.split('/'))
Out[44]: <generator object <genexpr> at 0x7fdf7411cd20>
In [45]: list(x('bin','*py*',lambda s:s.split('/')))
Out[45]:
[['bin', 'pythontex_engines.py'],
...,
['bin', 'pythontex2.py']]
In [46]:


As you have noticed, my code doesn't yield a file name but rather it returns a generator object... that's not exactly what you want but its use in a for loop is the same and other use cases can be easily taken into account.

### Another take at it

As the question was about proper indentation, I'm ambigously, ashamedly proud of this new attempt of mine...

def find(root, pattern, *callback):

from os.path import join
from os import walk
from fnmatch import fnmatch

gen = (join(cr, fn)

#   Trompe-l'oeil code

for cr, dl, fl in walk(root)
for fn in fl
if fnmatch(fn, pattern))
#               join(cr, fn) )

return (callback[0](fn) for fn in gen) if callback else gen