4
\$\begingroup\$

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
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ Feb 15, 2015 at 8:27

4 Answers 4

2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ This approach is to slow, generator expressions are far faster then for loops, Also they are more pythonic. \$\endgroup\$ Feb 13, 2015 at 17:12
  • 1
    \$\begingroup\$ 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... \$\endgroup\$
    – glglgl
    Feb 13, 2015 at 17:21
  • \$\begingroup\$ If space is not a constraint and speed is paramount, list comprehensions are usually faster than generators because generators generate a bit of overhead. \$\endgroup\$
    – gboffi
    Feb 13, 2015 at 18:14
  • \$\begingroup\$ @myself For large amount of data generators can be not only more efficient but also faster. \$\endgroup\$
    – gboffi
    Feb 13, 2015 at 18:23
  • 1
    \$\begingroup\$ @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 \$\endgroup\$
    – volcano
    Feb 13, 2015 at 22:49
1
\$\begingroup\$

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)
  1           0 LOAD_FAST                0 (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)
  2           0 LOAD_FAST                1 (callback)
              3 LOAD_FAST                0 (param)
              6 CALL_FUNCTION            1
              9 RETURN_VALUE        
>>> dis.dis(test2)
  2           0 LOAD_FAST                1 (callback)
              3 POP_JUMP_IF_FALSE       20

  3           6 LOAD_FAST                1 (callback)
              9 LOAD_CONST               1 (0)
             12 BINARY_SUBSCR       
             13 LOAD_FAST                0 (param)
             16 CALL_FUNCTION            1
             19 RETURN_VALUE        

  5     >>   20 LOAD_FAST                0 (param)
             23 RETURN_VALUE        
             24 LOAD_CONST               0 (None)
             27 RETURN_VALUE        
>>> dis.dis(test3)
  2           0 LOAD_FAST                1 (callback)
              3 LOAD_CONST               0 (None)
              6 COMPARE_OP               9 (is not)
              9 POP_JUMP_IF_FALSE       22

  3          12 LOAD_FAST                1 (callback)
             15 LOAD_FAST                0 (param)
             18 CALL_FUNCTION            1
             21 RETURN_VALUE        

  5     >>   22 LOAD_FAST                0 (param)
             25 RETURN_VALUE        
             26 LOAD_CONST               0 (None)
             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

\$\endgroup\$
0
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ 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): \$\endgroup\$
    – volcano
    Feb 13, 2015 at 21:48
  • \$\begingroup\$ @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. \$\endgroup\$ Feb 14, 2015 at 1:53
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

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

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