4
\$\begingroup\$

I'm a new programmer, who has just finished his first small project.

It's a sort of basic imitation of the grep command from Linux. I'm learning from projects the hard way. Here is the description of the project:

The first project that we’ll try is called logfind. It’s nothing more than a simple version of grep so you can learn a few simple basic things in Python:

  1. Opening files based on regular expressions in a file.
  2. Scanning those files for strings from command line arguments.
  3. Create a complete installable Python project.

When I run the script, the first thing it will look for islogfind.txt file on the user's computer, then it will scan this file according to a regular expression to search for filenames of the example.extension type. The files specified in the logfind.txt file will be searched for the strings that the user has specified on the command line.

I have a few questions about my program:

  • Could someone critique my code on style and readability?
  • Could you also check the overall design of the code and evaluate it?
  • Are there functions that's syntax can be shortened?

Source code:

# logfind.py
# Project explanation: http://projectsthehardway.com/
# A basic implementation of the linux grep command. The program will search for the given strings in files that are listed in a logfind.txt file. 
# By default the program will search for all the given strings in a file. If -o option is enabled, the program will return the file if one of the strings is present. 
# The results will be written to results.txt located in the current working directory.

import argparse
import os
import re

def cl_handler():
    """
    Handles the command line input, with strings as the words to search for in files, and -o as optional argument
    """

    parser = argparse.ArgumentParser(description="find strings inside files")
    parser.add_argument("strings", nargs = "*", help="The files will be searched according to this words")
    parser.add_argument("-o", action="store_true", help="This option resets the string1 AND string2 and string3 logic to a string1 OR string2 OR string3 search logic")
    args = parser.parse_args()
    return args.strings, args.o


def scan_logfind(logfind_dir):
    """
    Opens the logfind file and scans it for filenames according to a regular expression (filename.extension). 
    Returns a list with the filenames
    """
    files = []
    with open(logfind_dir, "r") as logfind:
        regex = re.compile(r"^[\w,\s-]+\.[A-Za-z]+$")  
        for word in logfind.read().split():            
            file = regex.match(word)
            if file:
                files.append(word)
    return files


def scan_directory(file):
    """
    Scans the computer for a specified file, starting with the home directory. 
    Returns the absolute directory of the file
    """
    home = os.path.expanduser("~")
    for root, dirs, files in os.walk(home):
        for f in files:
            if f == file:        
               file_directory = os.path.join(root, f)
               return file_directory


def search_strings(file_dir, strings, or_option=False):
    """
    Searches the file for the specified files. Returns boolean true if all strings are found in the file
    If the or_option is enabled the function will return boolean true if one string is found in the file.
    """
    with open(file_dir, "r") as logfile:
        logfile = logfile.read().lower()
        results = []
        for string in strings:
            if string in logfile:
                results.append("True")
            else:
                results.append("False")

    if or_option:
        for result in results:
            if result == "True":
                return True
        return False
    else:
        for result in results:
            if result == "False":
                return False
        return True


def main():
    """ 
    main 
    """
    results = open("results.txt", "w")
    strings, or_option = cl_handler()                 
    logfind = scan_directory("logfind.txt")          

    logfiles = scan_logfind(logfind)
    logfiles_dir = []        
    for logfile in logfiles:
        logfiles_dir.append(scan_directory(logfile))
    for logfile_dir in logfiles_dir:
            if search_strings(logfile_dir, strings, or_option):
                results.write("{}\n".format(logfile_dir))
    print("Search complete. Results written to results.txt")

if __name__ == "__main__":
    main()
\$\endgroup\$
4
\$\begingroup\$

Generators

Sometimes you make a list a piece at a time, to write a generator function you just yield the result each iteration and not build a list, the pattern:

result = []
for i in a_thing:
    result.append(do_thing(i))
return result

Must be avoided as it wastes memory and is boilerplate.

More concretely, you have:

def scan_logfind(logfind_dir):
    """
    Opens the logfind file and scans it for filenames according to a regular expression (filename.extension). 
    Returns a list with the filenames
    """
    files = [] # <- 1
    with open(logfind_dir, "r") as logfind:
        regex = re.compile(r"^[\w,\s-]+\.[A-Za-z]+$")  
        for word in logfind.read().split():            
            file = regex.match(word)
            if file:
                files.append(word) # <- 2
    return files # <- 3

You should write:

def scan_logfind(logfind_dir):
    """
    Opens the logfind file and scans it for filenames according to a regular expression (filename.extension). 
    Returns a generator with the filenames
    """
    ## Deleted -- files = [] # <- 1
    with open(logfind_dir, "r") as logfind:
        regex = re.compile(r"^[\w,\s-]+\.[A-Za-z]+$")  
        for word in logfind.read().split():            
            file = regex.match(word)
            if file:
                yield word # Gives this element out of the function.
    ## Deleted -- return files # <- 3

Ternary when sensible

Ternaries should not be overused, but can be a valuable tool to reduce code-bloat, see:

        if string in logfile:
            results.append("True")
        else:
            results.append("False")

The action append is the same, only the value changes, a perfect job for ternaries:

results.append("True" if string in logfile else "False")

But the whole top part of the function search_strings should be a generator expression as noted above.

Temporary overuse

You really like temporary variables, I would make less use of them, especially if they got confusing names:

file = regex.match(word) # This is not a filename nor a filecontent

Just write:

if regex.match(word):
    yield word

Another example:

       file_directory = os.path.join(root, f)
       return file_directory

This follows the:

result = computation(thing)
return result

Anti-pattern, just write:

return os.path.join(root, f)

A case analysis: search_strings

search_strings was the worst of your functions considering all the wheels that you reinvented there. Python has a ton of built-ins, please use them.

A first refactor was just:

  • Reducing nesting by closing the file early.
  • Using generator expressions.
  • Using the built-ins all and any

def search_strings(file_dir, strings, or_option=False):
    """
    Searches the file for the specified files. Returns boolean true if all strings are found in the file
    If the or_option is enabled the function will return boolean true if one string is found in the file.
    """
    with open(file_dir, "r") as f:
        logfile = f.read().lower()

    results = ("True" if string in logfile else "False" for string in strings)

    if or_option:
        return any(result == "True" for result in results)
    else:
        return all(result == "True" for result in results)

After:

  • Realizing you are using "True" instead of True for misterious reasons.
  • Using a ternary for more FP.

def search_strings(file_dir, strings, or_option=False):
    """
    Searches the file for the specified files. Returns boolean true if all strings are found in the file
    If the or_option is enabled the function will return boolean true if one string is found in the file.
    """
    with open(file_dir, "r") as f:
        logfile = f.read().lower()

    any_or_all = (any if or_option else all)
    return any_or_all(string in logfile for string in strings)

A FP purist would avoid all temporary variables and write it like:

def search_strings(file_dir, strings, or_option=False):
    """
    Searches the file for the specified files. Returns boolean true if all strings are found in the file
    If the or_option is enabled the function will return boolean true if one string is found in the file.
    """
    with open(file_dir, "r") as f:
        return (any if or_option else all)(string in f.read().lower() for string in strings) 

The second and third versions are more or less readable to different people, choosing between them is subjective, but surely any of them is better than your version (3-7 lines vs 20 of yours and are simpler then it).

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

You should parse command line inputs when you know you script is invoked from the command line. This allows you to more easily test your main function in the interactive interpreter and avoid importing argparse when it’s not needed. I’d rather write

if __name__ == "__main__":
    import argparse

    # Handles the command line input, with strings as the words to search for in files, and -o as optional argument
    parser = argparse.ArgumentParser(description="find strings inside files")
    parser.add_argument("strings", nargs = "*", help="The files will be searched according to this words")
    parser.add_argument("-o", action="store_true", help="This option resets the string1 AND string2 and string3 logic to a string1 OR string2 OR string3 search logic")
    args = parser.parse_args()

    main(args.strings, args.o)

and modify main accordingly. But reusing your function in the same kind of context is also valid:

def cl_handler():
    """
    Handles the command line input, with strings as the words to search for in files, and -o as optional argument
    """
    import argparse

    parser = argparse.ArgumentParser(description="find strings inside files")
    parser.add_argument("strings", nargs = "*", help="The files will be searched according to this words")
    parser.add_argument("-o", action="store_true", help="This option resets the string1 AND string2 and string3 logic to a string1 OR string2 OR string3 search logic")
    args = parser.parse_args()
    return args.strings, args.o

if __name__ == "__main__":
    main(*cl_handler())

Also note that

"""
main
"""

serves absolutely no purpose. It has no added value and is just noise. Either write a proper docstring or write nothing at all.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It is a convention to group imports at the top to know what is being used in the file at a first glance. argparse is in the standard library, so there is no harm in importing it every time. \$\endgroup\$ – Caridorc Nov 5 '15 at 15:00
  • \$\begingroup\$ @Caridorc I’m aware of that convention and usually follow it, but for the case of argparse I tend to only include it in the if __name__ == "__main__" part. I mean, it has absolutely no use otherwise and serves only to bootstrap calls in this portion of the code, so why bother when it’s not needed? I can't find anything to reinforce that claim, though, so I might want to reconsider it. Note, however, that the point of the answer was also (mainly?) to reconsider the coupling between main and argparse and remove the need of the latter when executing the former. \$\endgroup\$ – 301_Moved_Permanently Nov 5 '15 at 15:17
  • \$\begingroup\$ stackoverflow.com/questions/128478/… is a nice discussion in the topic, anyhow in this case import argparse may live anywhere, it is minor. \$\endgroup\$ – Caridorc Nov 5 '15 at 15:21
2
\$\begingroup\$

If this is your first project, then you are off to a good start! I see many commendable qualities:

  • Functionality is logically organized into functions, with no free-floating code outside of functions
  • Following the if __name__ == "__main__" convention
  • Docstrings (Please keep the line length under 72, though!)
  • Using argparse instead of reinventing the wheel

Spec compliance

Why are you recursively scanning my entire home directory for files named logfind.txt? Some people's home directory can be huge. The project description just says I specify what files are important in a ~/.logfind file, using regular expressions.

Why do you write your results to results.txt? That's unexpected — conventionally, results would just be printed to sys.stdout. You could be overwriting my valuable data. If you do write to results.txt, at least open it with the x flag to ensure that you don't overwrite anything. Also note that you neglected to use a with block when opening results.txt as you did in scan_logfind().

search_strings()

Why is the first parameter named file_dir? It's supposed to be a text file, not a directory.

Using special strings "True" and "False" is weird. Why not use booleans True and False?

It's bad practice to reassign logfile = logfile.read().lower() in a way that changes its type from a file handle to a string.

The exercise encourages you to write a fast solution. The costliest operations in this problem are ones involving disk I/O. Specifically, you should try to avoid reading the entire file at once with logfile.read(), because:

  • It scales poorly if logfile is large.
  • You may be able to avoid reading the rest of the file if the keywords occur early in the file.

I'd rewrite search_strings() like this:

def file_contains_strings(file_path, strings, conjunction=all):
    """Checks whether the file contains the specified strings.

    The conjunction should be either the builtin function all()
    (the default) or any().
    """
    search = { text.lower(): False for text in strings }
    with open(file_path) as f:
        for line in f:
            line = line.lower()
            for text, found in search.items():
                search[text] = found or (text in line)
            if conjunction(search.values()):
                return True
    return False

Instead of passing or_option, the caller specifies all or any, which is more self-documenting (and it also simplifies the implementation of this function). I've renamed the function as a predicate rather than a procedure, so that the code at the call site reads better:

if file_contains_strings(logfile, strings, any if or_option else all):
    …

Note that lowercasing the text is technically not the right way to do case-insensitive search.

| improve this answer | |
\$\endgroup\$

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.