current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

This is a improved version of the program seen in my older question.

This program first displays a menu which shows some options that the user can select. Each option will the respective functions which do some things.

The user can insert a filename, read data from it, enter a regex pattern, test if each line of the file matches the given regex pattern.

Please provide feedback on how I can improve my program.

import re


def test_data(readData, regex):
    '''Checks if each line of data matches the regex'''

    if not regex:
        print('Empty regex')
        print('')
        return

    try:
        pattern = re.compile(regex)
    except:
        print('Invalid regex')
        print('')
        return

    if not readData:
        print('Read data is empty')
        print('')
        return

    for i, line in enumerate(readData, 1):
        line = line.strip()  #Strip off the newline character from each line
        if pattern.match(line):
            print(' Line ', i, ':"', line,'"', ' matches current regex', sep='')
        else:
            print(' Line ', i, ':"', line,'"', ' does not match current regex', sep='')
    print('')

def change_regex(regex):
    '''Sets the regex variable'''

    regex = '"%s"' % regex if regex else 'empty'
    print('Current regex is', regex)

    regex = input('Enter new regex: ')
    print('')
    return regex

def read_file(filename):
    '''Reads data from file if possible'''

    try:
        with open(filename, "r") as file:
            readData = file.readlines()
            print('Data read successfully!')
            return readData
    except TypeError:
        print('filename is empty!')
    except FileNotFoundError:
        print('File not found!')

    print('')

def change_filename(filename):
    '''Sets the filename variable'''

    filename = '"%s"' % filename if filename else 'empty'
    print('Current filename is', filename)

    filename = input('Enter new filename (with extension): ')
    print('')
    return filename

def main():
    filename = regex = readData = None
    while True:
        print('Enter 1 to change filename')
        print('Enter 2 to read file')
        print('Enter 3 to change regex')
        print('Enter 4 to test if read data matches given regex')
        print('Enter 5 to exit')

        try:
            option = int(input('Option: '))
        except:
            print('Invalid input')
            print('Please enter a valid option')
            print('')
            continue

        if option == 1:
            filename = change_filename(filename)
        elif option == 2:
            readData = read_file(filename)
        elif option == 3:
            regex = change_regex(regex)
        elif option == 4:
            test_data(readData, regex)
        elif option == 5:
            break
        else:
            print('Invalid input')
            print('Please enter a valid option')
            print('')

if __name__ == "__main__":
    main()
share|improve this question

1 Answer 1

up vote 0 down vote accepted

Pure functions

All of your functions both "do work" and print. You should separate logic and In/Out.

print('') is poor practice

If you want an extra newline just concat it to the previous message:

print('Empty regex\n')
return

Avoid noise

Maybe in a file of 1000 lines, my regex will match 5 lines, why must I watch 995 useless lines of output? Just output lines where it does match.

Prefer a dictionary or indexes to an array to a long if elif chain

if option == 1:
    filename = change_filename(filename)
elif option == 2:
    readData = read_file(filename)
elif option == 3:
    regex = change_regex(regex)
elif option == 4:
    test_data(readData, regex)
elif option == 5:
    break
else:
    print('Invalid input')
    print('Please enter a valid option')
    print('')

In all sincerity I don't know how to do this in this particular case, but iit would also give the benefit of generating the printing message on the fly, instead of repeating the same thing twice.

Command-line?

But maybe all of that effort for the menu is counterproductive and you should allow the user to call the programme like:

python regex_script.py <regex> <filename>

by using sys.argv or argparse.

My take

This is my implementation of this program, it separates Input/Output and logic and can be used concisely from the command line:

import re
import sys

def lines_matched(regex, text):
    for line in text.splitlines():
        if re.search(regex, line):
            yield line

def main():
    if len(sys.argv) != 3:
        print("Usage: python {} <regex> <filename>".format(__file__))
        return 1
    regex, filename = sys.argv[1], sys.argv[2]
    with open(filename) as f:
        print('\n'.join(lines_matched(regex, f.read())))

if __name__ == "__main__":
    main()

argparse would make it more user friendly.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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