I am searching in files with extensions .xml, .java, and .properties strings that match a certain regular expression.

When I find it, I write it to the file with the filename, line and string information.

def search(param):
    filename = "search-result.txt"
    except OSError:
    os.path.walk(param, step, None)

def step(ext, dirname, names):
    output = open("search-result.txt", "a")
    for name in names:
        if name.lower().endswith(".xml") or name.lower().endswith(".properties") or name.lower().endswith(".java"):
            path = os.path.join(dirname, name)
            filters = ["\\bin\\", "\\build\\", "logback", "\\test\\", "\\target\\"]
            if all(not filter in path for filter in filters):
                with open(path, "r") as lines:
                    print "Read: {}".format(path)
                    i = 1
                    for line in lines:
                        m = re.search(r"(!|$|RUP)\{[^:]*:[^\}]*\}", line)
                        if m is not None:
                            output.write("Path: {0}; \n    Line number: {1}; \n        {2}\n".format(path, i, line))
                        i += 1

I divided it into two functions - search, where I can check where result file is existed and remove it, and then step for os.path.walk. Param is a folder to search in.

How can this code be adjusted to look better?


1 Answer 1


These parameter names are confusing: param and ext.

The step() function suffers from excessive nesting. I would further subdivide your step() function to create a search_path() function that deals with each candidate file. Since both of these helper functions aren't really that useful on their own, I would define them both within the main search() function.

You've hard-coded "search-result.txt" twice. Ideally, it should be parameterized rather than hard-coded at all. Furthermore, you're reopening it for appending each time you enter a directory, which is problematic because…

  • Reopening the file handle is wasteful.
  • You might not even have any search results for that directory.
  • If you opened it just once in 'w' mode instead of many times in 'a' mode, you wouldn't have to remove the file at all. (Note that removing the file does make a difference, if the file exists and has a second hard-link.)

Also, if all(not filter in path for filter in filters) is inefficient. As explained in the documentation for [os.path.walk()], you can avoid entering directories in which you have no interest:

The visit function may modify names to influence the set of directories visited below dirname, e.g. to avoid visiting certain parts of the tree. (The object referred to by names must be modified in place, using del or slice assignment.)

To analyze file extensions, use os.path.splitext().

Idiomatic Python loops rarely need statements like i += 1. What you want to use is enumerate().

The format string for output.write() is easier to read if spread out into several lines.

import os.path
import re

EXCLUDE_DIRS = ['bin', 'build', 'logback', 'test', 'target']

def search(directory, output_filename='search-result.txt'):
    def search_path(output, path):
        """Output search results for one file"""
        with open(path) as lines:
            print "Read: {}".format(path)
            for i, line in enumerate(lines, 1):
                if re.search(r"(!|$|RUP)\{[^:]*:[^\}]*\}", line):
                    output.write("Path: {0}; \n"
                                 "    Line number: {1}; \n"
                                 "        {2}\n".format(path, i, line))

    def step(output, dirname, names):
        """Filesystem traversal excluding certain directory names,
           calling search_path() for candidate files"""
        names[:] = [entry for entry in names if entry not in EXCLUDE_DIRS]
        for name in names:
            _, ext = os.path.splitext(name)
            if ext.lower() in ('.xml', '.properties', '.java'):
                search_path(output, os.path.join(dirname, name))

    with open(output_filename, 'w') as output:
        os.path.walk(directory, step, output)

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.