5
\$\begingroup\$

Please bear with me, I'm only about a week into Python.

This code is something that I wanted to do once I completed the chapters dealing with opening and reading files, and RegEx. I always feel like I'm over complicating the code. Please take a look and let me know if this is sound and where it can be improved. There are a lot of concepts I haven't learned yet. I'm teaching myself out of a book.

This code is meant to open a log file (will eventually be retrieved real-time), go through it line by line using RegEx to find "Fail", add those lines reporting failures to a list and return the completed list as a string joined with new lines.

#!/usr/bin/python

import os
import re

myFile = '/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt'

def RegEx(file):
    openFile = open(myFile, 'r')
    myResList = []
    myList = openFile.readlines()
    myReg = re.compile(r'Fail')
    for i in (myList):
            #print(i, end='')
            result = myReg.search(i)
            if result != None:
                    #print(i, end='')
                    myResList.append(i.rstrip())
    final = '\n'.join(myResList[:])
    return final

print()
print('Results:')
print(RegEx(myFile))
print()
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! As a courtesy to others, please declare your cross-posts. \$\endgroup\$ – 200_success Jan 19 '16 at 1:13
10
\$\begingroup\$

The standard indentation for Python, specified by PEP 8, is four spaces. This is a pretty strong convention for Python, since indentation matters a lot.

You almost always want to call open() in the context of a with-block, so that the file will get closed automatically when exiting the block.

This task does not require you to read the entire file at once. I recommend working line by line so that you can handle large files better. (You also don't need to make a copy of myResList at the end using myResList[:].)

You don't need a regular expression to search for a fixed literal string. A substring search will do just fine. This also means that your function is misnamed. In general, you should name functions after the task that they accomplish, rather than how they accomplish the task.

I'd generalize your function to search for any text.

def search(filename, text):
    with open(filename) as f:
        for line in f:
            if text in line:
                print(line)

if __name__ == '__main__':
    search('/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt', 'Fail')

If you're interested more in passing back the results, a better way would be to yield the results back to the caller.

def search(filename, text):
    with open(filename) as f:
        for line in f:
            if text in line:
                yield line

if __name__ == '__main__':
    for result in search('/home/nick/python/chap8ReadingWritingFiles/15Jan2016_000000.txt', 'Fail'):
        print(result)

The if __name__ == '__main__': boilerplate is optional, but considered standard practice.

\$\endgroup\$
  • \$\begingroup\$ Thank you! Just a couple of things. Eventually this will have to filter out complicated strings that aren't literal, and add the results to multi-latered dictionaries and lists, that's why i kept it RegEx. Also, you suggested going through the file line by line, but I thought that's what my code did with the readlines(), is that not the case? I don't see any read() or readlines() in your code, I was under the impression that was a requirement to read from any file. I haven't learned "with" or "yield" yet, but I will seek that info out. Thanks very much for your helpful advice! \$\endgroup\$ – user95110 Jan 19 '16 at 17:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy