2
\$\begingroup\$

For an assignment, I had to loop through a text file, and report any positions that a user entered sequence was found. Is there any way I can make this code more efficient, because it seems like I'm writing a lot of nested loops, which I've learned can be dangerous/complicated. Any and all help is appreciated!

import os #OperatingSystem
import sys #System
import re #Regex (Regular Expressions)

def checkSequenceCharacters(seq):
    for c in seq:
        if c != 'A' and c != 'G' and c != 'T' and c != 'C':
            print("[!] ERROR: Not a valid sequence (A/G/T/C ONLY)")
            sys.exit(0)

def checkSequenceLength(seq):
    if len(seq) != 5:
        print("[!] ERROR: Has to be exactally 5 letters long!")
        sys.exit(0)

seq = raw_input("Enter a 5 basepair sequence using only A,G,T,C: \n")

checkSequenceCharacters(seq)
checkSequenceLength(seq)

input_file_path = os.getenv("HOME") + "/Desktop/sequencer/inputfile.txt"
output_file_path = os.getenv("HOME") + "/Desktop/sequencer/output/" + seq + ".txt"

try:
    input_file = open(input_file_path)
    output_file = open(output_file_path, 'w')
    count = 0
    line_location = 0
    lines = []
    for line in input_file:
        line_location = line_location + 1
        arr = re.findall(seq,line)
        if arr:
            x = arr[0]
            if x == seq:
                count = count + 1
                lines.append(line)
                print("[+] Sequence found at line " + str(line_location))
                output_file.write("Line " + str(line_location) + ": " + line + "\n")
    if count != 0:
        print("[*] Matches: " + str(count))
        print("[*] FILE CREATED AT: " + output_file_path)
    else:
        print("[!] ERROR: No matches found!")
finally:
    input_file.close()
    output_file.close()
\$\endgroup\$
  • \$\begingroup\$ Any chance that a sequence may cross a line boundary? \$\endgroup\$ – vnp Jan 9 at 23:57
4
\$\begingroup\$

TL;DR: See end of answer for updated code with suggested improvements

While I don't have many suggestions regarding nested for-loops, as you only seem to have two of them, I do have a few other suggestions:

Suggestions

1. Use with for file opening

You use try/finally to open/close your files, and while you're absolutely right to close your files, Python's with statement takes care of this for you. Another lesser-known characteristic of the with statement is that you can "nest" them in a single line, which is useful in your case.

So, I would recommend updating

try:
    input_file = open(input_file_path)
    output_file = open(output_file_path, 'w')
    ...  # Do stuff
finally:
    input_file.close()
    output_file.close()

to

with open(input_file_path) as input_file, open(output_file_path, "w") as output_file:
    ...  # Do stuff

When the context of the with statement is exited, your opened files will automatically be closed.

2. Clean up variable naming

Python convention is to use lower snake-case for normal variable names and function names - Camel-case (upper) is usually only used for class definitions. Therefore, I would recommend renaming your checkSequenceCharacters and checkSequenceLength functions to check_sequence_characters and check_sequence_length, respectively. This might be considered nitpicking, but maintaining consistent naming conventions always makes things easier for me.

3. Use str in str, instead of checking individual equality

In check_sequence_characters, you can more easily ensure that each character in seq is in an allowed set of characters using the following:

if any(c not in "AGTC" for c in seq):
    raise ValueError("Not a valid sequence (A/G/T/C ONLY)")

4. Raise Exceptions, instead of printing "ERROR"

There are a few places in your example where you print error messages. That's fine if you really don't want to raise actual exceptions, but that does seem to be the behavior your code is dancing around. I would recommend reading Python's documentation on raise if you're unfamiliar with it.

5. Use if __name__ == "__main__" when running module as source

If you don't know what if __name__ == "__main__" means, check out this explanation. Basically, my recommendation is to start using this pattern even if it may not be strictly necessary in your example (although it does protect you from the lurking seq name-shadowing bug). This will facilitate maintenance of your code should you need to import your example code as a module in the future, for example.

6. Use enumerate to loop through input_file with line index

Instead of using the line_location variable, you can remove the line_location = 0 line, and change

for line in input_file:
    line_location = line_location + 1

to

for line_location, line in enumerate(input_file):

If you still want line_location to be 1-indexed, just add 1 when you print/write it below.

Other Notes

  • For more recent versions of Python, you'll need to use input instead of raw_input
  • If there is any chance your output file has not already been created by you (which seems likely), you should open it using mode="x", which will create and open the file
  • If any of the subdirectories in the output file path have not already been created, you'll need to use something like os.makedirs to handle directory creation
  • The line print("[*] FILE CREATED AT: " + output_file_path) near the bottom may be misleading, as the output file is created when you call open, not when you call write. This means that even if count == 0, an output file was created (albeit a blank one)
  • In the provided suggested code, I removed the lines variable, since it isn't used; however, if you actually do need it for something not shown in your example, it can be safely re-added, along with the lines.append(line) line
  • I would recommend embracing Python's string format method, instead of using + to construct strings

TL;DR

If you decide to follow the above suggestions, your code will probably end up looking something like this:

import os
import re

def check_sequence_characters(seq):
    if any(c not in "AGTC" for c in seq):
        raise ValueError("Not a valid sequence (A/G/T/C ONLY)")

def check_sequence_length(seq):
    if len(seq) != 5:
        raise ValueError("Has to be exactly 5 letters long!")

def _execute():
    seq = raw_input("Enter a 5 basepair sequence using only A,G,T,C: \n")
    count = 0
    check_sequence_characters(seq)
    check_sequence_length(seq)
    input_file_path = os.getenv("HOME") + "/Desktop/sequencer/inputfile.txt"
    output_file_path = os.getenv("HOME") + "/Desktop/sequencer/output/" + seq + ".txt"

    with open(input_file_path) as input_file, open(output_file_path, 'x+') as output_file:
        for line_location, line in enumerate(input_file):
            arr = re.findall(seq, line)

            if arr and arr[0] == seq:
                count += 1
                print("[+] Sequence found at line " + str(line_location + 1))
                output_file.write("Line " + str(line_location + 1) + ": " + line + "\n")

        if count != 0:
            print("[*] Matches: " + str(count))
            print("[*] FILE CREATED AT: " + output_file_path)
        else:
            raise RuntimeError("No matches found!")

if __name__ == "__main__":
    _execute()
\$\endgroup\$
  • 2
    \$\begingroup\$ enumerate takes as an optional second argument the number to start counting from. So if you want 1-indexed you can just do for i, line in enumerate(input_file, 1):. \$\endgroup\$ – Graipher Jan 10 at 6:36
  • \$\begingroup\$ @Graipher, thanks for bringing that up! I completely forgot about it. \$\endgroup\$ – Hunter McGushion Jan 10 at 8:58

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.