Automate the Boring Stuff Chapter 9 - Regex Search

Question

The project outline:

Write a program that opens all .txt files in a folder and searches for any line that matches a user-supplied regular expression. The results should be printed to the screen.

My solution:

import os, re
from pathlib import Path

while True:
    user_path = input("Enter a path to search: ")
    if user_path == "":
        print("Please enter a valid path.")
    elif Path(user_path).exists() != True:
        print("This path does not exist.")
    else:
        while True:
            all_files = os.listdir(user_path)
            matches = []
            lines = []
            user_input = input("What do you want to find? (Enter nothing to search another path)\n")
            regex_object = re.compile(user_input)
            if user_input == "":
                break
            else:
                for filename in all_files:
                    if filename.endswith(".txt") != True:
                        continue
                    text_file = open(Path(user_path) / filename)
                    for line in text_file.readlines():
                        match_object = regex_object.findall(line)
                        for match in match_object:
                            matches.append(match)
                            lines.append(line)
                    text_file.close()
            if matches == []:
                print("No matches found")
            else:
                print(matches)
                print(lines)

Thanks in advance for the critique.

Answer 1

Use stronger input validation

The code checks:

if Path(user_path).exists() != True:

And later it does:

all_files = os.listdir(user_path)

Also later it creates another Path from the same path string:

open(Path(user_path) / filename)

It would be better to check that the input is a directory, and create a Path object once and reuse it. So the above uses become:

basedir = Path(user_path)
# ...

if not basedir.is_dir():
    # ...

all_files = os.listdir(basedir)
# ...

open(basedir / filename)

Wrap code in if __name__ == '__main__': guard

The recommended way to organize Python code is put the main logic in functions, and call the functions from under a if __name__ == '__main__': guard, something like this:

def main():
    # the main work, probably calling other functions
    # ...

if __name__ == '__main__':
    main()

Use functions

It's good to organize code to functions, where each function does one logical thing.

For example the innermost part of the code could be in a function:

def find_matches_and_lines(basedir, re_pattern):
    matches = []
    lines = []
    for filename in os.listdir(basedir):
        if not filename.endswith(".txt"):
            continue
        with open(basedir / filename) as text_file:
            for line in text_file.readlines():
                for match in re_pattern.findall(line):
                    matches.append(match)
                    lines.append(line)

    return matches, lines

The benefit is that a smaller piece code like this is easier to understand as a single logical unit, without having to worry about all the complexity in the rest of the program outside of this function.

Zip the results

The program builds two parallel lists: matches and the matched lines. I don't know how you plan to use it. I think it's very likely that when looking at a match, you might want to look at the corresponding line. With two parallel lists, you have to use indexes. Which is ok, but if you only plan to use the values in pairs, then it would make sense zip the lists, which would open up the possibility to use a generator pattern, and a bit simpler code:

def find_matches_and_lines(basedir, re_pattern):
    for filename in os.listdir(basedir):
        if not filename.endswith(".txt"):
            continue
        with open(basedir / filename) as text_file:
            for line in text_file.readlines():
                for match in re_pattern.findall(line):
                    yield match, line

Simplify conditionals on empty things

PEP8 recommends to use the fact that empty sequences are false. That is, instead of these:

if user_path == "":

if matches == []:

Write:

if not user_path:

if not matches:

And instead of these:

elif Path(user_path).exists() != True:

if filename.endswith(".txt") != True:

Write:

elif not Path(user_path).exists():

if not filename.endswith(".txt"):

Use context manager for file operations

Instead of:

text_file = open(path, "r")
text = text_file.read()
text_file.close()

The recommended idiom:

with open(path, "r") as text_file:
    text = text_file.read()

And then no need to call .close() explicitly, the context manager will do it automatically when leaving the scope of the with.