6
\$\begingroup\$

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

filename = None
readData = None
regex    = None

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

    global readData
    global regex

    if regex == None:
        print('Empty regex')
        print('')
        return None

    try:
        pattern = re.compile(regex)
    except:
        print('Invalid regex')
        print('')
        return None
    i = 1
    if readData == None:
        print('Read data is empty')
        print('')
        return None

    for line in readData:
        if pattern.match(line.strip()):
            print(' Line ', i, ':"', line,'"', ' matches current regex', sep='')
        else:
            print(' Line ', i, ':"', line,'"', ' does not match current regex', sep='')
        i += 1
    print('')

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

    global regex
    print('Current regex is ', end='')
    if regex != None:
        print('"', regex, '"', sep='')
    else:
        print('empty')

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

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

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

    print('')

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

    global filename
    print('Current filename is ', end='')
    if filename != None:
        print('"', filename, '"', sep='')
    else:
        print('empty')

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

def show_menu():
    '''Shows the menu'''

    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')

def main():
    while True:
        show_menu()
        try:
            option = int(input('Option: '))
        except:
            print('Invalid input')
            print('Please enter a valid option')
            print('')
            continue
        if option == 1:
            change_filename()
        elif option == 2:
            read_file()
        elif option == 3:
            change_regex()
        elif option == 4:
            test_data()
        elif option == 5:
            break
        else:
            print('Invalid input')
            print('Please enter a valid option')
            print('')

if __name__ == "__main__":
    main()
\$\endgroup\$
  • \$\begingroup\$ You can find some ideas here codereview.stackexchange.com/questions/93939/… \$\endgroup\$ – outoftime Jun 20 '15 at 8:58
  • \$\begingroup\$ @outoftime , Thanks. But shouldn't the the post be rolled back to revision 2? \$\endgroup\$ – Spikatrix Jun 20 '15 at 9:11
  • \$\begingroup\$ Sorry, I didn't understand, what are you talking about? What post, what revision? \$\endgroup\$ – outoftime Jun 20 '15 at 9:23
  • \$\begingroup\$ @outoftime, OP changed the code and made answers meaningless. Shouldn't the change be reverted? \$\endgroup\$ – Spikatrix Jun 20 '15 at 9:23
  • \$\begingroup\$ Not really. All I sad in that answer is good to know in any case. \$\endgroup\$ – outoftime Jun 20 '15 at 9:32
8
\$\begingroup\$

Some thoughts:

  1. Don't use globals, pass variables as function arguments.
  2. Follow pep8
  3. Don't use == None. False, 0, 0.0, None, and empty sequences like lists or strings are all considered False, so in your case you can just do something like if not regex. If you need to test for None specifically, use is None.
  4. Don't do return None, just an empty return will return None automatically.
  5. Don't do a blind except:, you should always check for a specific exception.
  6. For the loop, use enumerate to keep track of the current index.
  7. Use with open(filename, 'r'):. This will automatically close the file when you leave the with block, even if there is an exception, so you don't need to manually close.
  8. In read_file, don't catch the exception at all. The default exceptions give the user more information and more useful information than your exceptions, so just let them be handled normally.
  9. It is clearer to do something like print('"%s"' % regex) rather than print('"', regex, '"'). Or better yet regex = '"%s"' % regex if regex else 'empty', then print('Current regex is', regex).
  10. show_menu shouldn't be a separate function.
\$\endgroup\$
  • 5
    \$\begingroup\$ Don't use globals, pass variables as function arguments. this is pure gold, sadly it also is hard to understand for beginners. \$\endgroup\$ – Caridorc Jun 19 '15 at 12:01
  • \$\begingroup\$ It is no different than anything else. It is like a if block, a for block, a def block, or any other similar block of code. You can (but usually shouldn't) put the whole thing in a try...except block, or (better) put the try...except block inside the with block. Edit: The question I am answering seems to have disappeared. It was about how to use with blocks with exception handling. \$\endgroup\$ – TheBlackCat Jun 19 '15 at 13:13
  • \$\begingroup\$ Just googled that and found it. Thanks anyway \$\endgroup\$ – Spikatrix Jun 19 '15 at 13:13
  • \$\begingroup\$ That is fine, but next time rather than deleting the comment please put a new comment saying that you have found the answer. \$\endgroup\$ – TheBlackCat Jun 19 '15 at 13:14

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.