5
\$\begingroup\$

This is my first time requesting a code review. This is code I wrote in Python to create a DNA sequence analyzer. This is for the Harvard CS50 course.

I was really hoping to get some constructive criticism on this code I finished for the DNA problem in Problem Set 6. This code passed all tests and is fully functional as far as I am aware.

I was mainly wondering if there were any more succinct ways to rewrite parts of it. I spent several hours writing this and my brain is pretty exhausted, so near the end I just went for whatever worked.

Any input is appreciated! (Also, sorry if this is an excessive amount of comments, but they help me keep everything clear in my head.)

import re
import csv
from sys import argv

# Checks for correct number of command-line arguments
if not len(argv) == 3:
    print("Incorrect number of command-line arguments.")
    exit(1)

# Declares a dictionary containing STR counts
str_counts = {
    "AGATC": "0",
    "TTTTTTCT": "0",
    "AATG": "0",
    "TCTAG": "0",
    "GATA": "0",
    "TATC": "0",
    "GAAA": "0",
    "TCTG": "0"
}

# Opens the csv and txt files to read (and closes them later)
with open(argv[1], "r", newline="") as csv_file, open(argv[2], "r") as sequence:
    # Reads the database into a dict and the sequence into a string
    db = csv.DictReader(csv_file)
    sq = sequence.read()

    # Stores the fieldname of the dictionary and stores the keys in STRS, skipping first line
    keys = db.fieldnames
    key_len = len(keys)
    # Skips first row of headers
    STRs = keys[1:key_len]

    # Bool to signal if match was found
    matched = False

    # Checks for STR matches and length of those matches, then stores those values in str_counts
    for STR in STRs:
        # Executes following code only if there are 1 or more matches
        if re.search(rf"(?:{STR}+)", sq):
            # Creates a list of matches
            matches = re.findall(rf"(?:{STR})+", sq)
            # Finds the longest match
            longest = max(matches, key = len)
            # Stores that value in corresponding dict key
            str_counts[STR] = len(longest) / len(STR)
            # Compares str_counts values to database to find match

    # Compares the match count values to the database values
    for row in db:
        # Declares a counter to later check if all STR values are matched
        # Resets counter to zero every iteration
        match_count = 0
        # Declares a temporary dictionary with only int match values to compare STR counts to
        compare = {}
        # Stores the names field for use later
        match_name = row['name']
        # Deletes the names field so we only have ints in our compare dict
        del row['name']
        # Converts values to integers for easy comparison
        for key, value in row.items():
            compare[key] = int(value)
        # Increments match count
        for STR in STRs:
            if compare[STR] == str_counts[STR]:
                match_count += 1
        # If all fields are matched, print match name, switch bool to true
        if match_count == len(STRs):
            matched = True
            print(match_name)

    if not matched:
        print("No Match")
\$\endgroup\$
  • 3
    \$\begingroup\$ Do you have any link to the problem description? because a DNA sequence analysis may have various purposes. Apart from that, there are some minor possible modifications but they would make the code quite compact such as replacing initialized and called variables with the initializer value. e.g matches = re.findall(rf"(?:{STR})+", sq) | longest = max(matches, key = len) => longest = max(re.findall(rf"(?:{STR})+", sq), key = len). \$\endgroup\$ – Miguel Avila Aug 7 at 3:14
  • 1
    \$\begingroup\$ Is counting all it has to do? We can shorten it quite a bit using I built-in tools then. Please post the description of the assignment/challenge. \$\endgroup\$ – Mast Aug 7 at 4:31
  • 3
    \$\begingroup\$ Probably this is the link to the problem description. \$\endgroup\$ – dariosicily Aug 7 at 6:11
  • 1
    \$\begingroup\$ @MiguelAvila thank you for the tip! I changed that in my code. \$\endgroup\$ – Star Aug 7 at 17:42
  • 2
    \$\begingroup\$ Hey, on Code Review it's site policy to leave the code in the question as is once you have gotten an answer. This rule helps prevent some very messy situations. As such I have rolled back your latest edit; I hope you can be understanding in this. You may be interested in our guidance on how best to follow up from getting an answer. \$\endgroup\$ – Peilonrayz Aug 7 at 18:50
6
\$\begingroup\$

I would group the code in functions and call them from a central main. With how you have it now, simply loading the file into an interactive console will cause all the code to run, which, if the code is long running or causes side-effects, may be less than ideal. Say I wrote a port-scanner, and it takes 2-3 minutes to run. I may not want it to start simply because I loaded the file into a REPL in Pycharm. Not only would that waste a few minutes each time, port-scanning can get you in trouble if done on a network that you don't have permission to scan.

Code wrapped in functions is simply easier to control the execution of, because it will only run when the function is called.

Grouping code into functions also allows you to easily test each chunk in isolation from the rest of the code. It's difficult to test code when you're dependent on the first half of the code to supply test data to the latter-half. It's much easier however to simply load the script into a REPL, and throw test data at the function (or use proper unit-tests). Then you can test one piece of code in isolation without touching the rest of the code.


I personally believe that STRs and STR should be lower-case. According to PEP8, Python's style and convention guide, regular variables should be lower case, with words separated by underscores. Variables that are considered to be constants however should be upper-case, separated by underscores.

Local variables that remain unchanged have never clicked as "constants" in my head, so it depends on what you decide.

Regardless though, STRs should either be all upper-case, or all lower-case; in which case the STR counterpart will need to be renamed so it doesn't clash with the str built-in. A name like str_ could be used, or you could use a more descriptive name.


compare = {}

. . .

for key, value in row.items():
    compare[key] = int(value)

This can make use of a dictionary comprehension. Whenever you have the pattern of creating a set/list/dictionary before a loop, then populating the structure within the loop, you should consider a comprehension:

compare = {key: int(value) for key, value in row.items()}

Similarly, to ensure that a condition holds for an entire collection (or two in this case), you can combine all with a generator expression (the part passed to all):

if all(compare[STR] == str_counts[STR] for STR in STRs):
    matched = True
    print(match_name)

"If all corresponding values in the two dictionaries match, set the matched flag". It reads quite nicely.

That gets rid of the need for match_count, and the second last loop.


You're looping more than you need to. You appear to only care about if at least one match is found; yet you keep looping even after one is.

I'd break once one is found. If you combine that with for's else clause, you can prevent unnecessary looping and still detect failure:

for row in db:
    . . .
    if all(compare[STR] == str_counts[STR] for STR in STRs):
        print(match_name)
        break

else:
    print("No Match")
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ With how you have it now, simply loading the file into an interactive console will cause all the code to run, which makes working with a REPL more difficult. So what about running python script from console with -i flag to keep it interactive? \$\endgroup\$ – Konstantin Kostanzhoglo Aug 7 at 17:11
  • 1
    \$\begingroup\$ @KonstantinKostanzhoglo I commonly use that and having no functions is hard to work with. \$\endgroup\$ – Peilonrayz Aug 7 at 17:14
  • \$\begingroup\$ @carcigenicate You are right, I just wanted to mention the possibility of being able to access the result of the computation interactively when the program finished its execution. \$\endgroup\$ – Konstantin Kostanzhoglo Aug 7 at 17:37
  • 2
    \$\begingroup\$ @Star No problem. And yes, when code is initially formatted to use globals, it can take some work to get everything arranged into functions. Ideally, every function should take as arguments the data it needs, and should return the results of whatever computations it does (ideally). That can make reasoning about code easier, and makes testing much easier, but ya, it takes some work to get there. For the last issue, make sure the else is properly indented at the same level as the for. Because else is valid for a few constructs, it's easy to "attach" it to the wrong statement. \$\endgroup\$ – Carcigenicate Aug 7 at 17:43
  • 1
    \$\begingroup\$ @Star Please do not update code in questions. If anyone is in the process of writing a review, a change to the code may invalidate what they have, and we certainly don't want to discourage any other suggestions. It's completely appropriate to post a second question with updated code if you want a review of the new code. If you post a new question, I may have time later to look it over. \$\endgroup\$ – Carcigenicate Aug 7 at 18:51
3
\$\begingroup\$

I'll start off by saying, Hey! Good job! There's a lot of things you got right in this code, including the use of with for file open/close operations and checking the number of command line arguments.

Now I'll rip it to shreds. :|

  1. First, check out PEP-8, the Python community coding standard.

    You may find things in there you disagree with, but it's a pretty good starting point. (And if you don't match it, everyone on here will nag you for it, so ...)

  2. Don't import sys.argv.

    This is a small thing, but other coders expect to see it spelled out. You can create a local variable that references it, if you're going to do a lot of operations. But for your purposes, it's actually more clear to write:

     if len(sys.argv) != 3:
    
  3. Don't declare your str_counts dictionary globally.

    Global variables are bad, m'kay? Also, there's no need for this, which I will get to later.

  4. Don't initialize your str_counts dictionary with data.

    I know why you did this, but you had to read the CSV file to get the keys so you could set this up in advance, right? And there's a couple of ways to do this all-at-once, which I will get to later. So just delete this entire global variable.

  5. Don't let your with blocks get too long.

    You have two items in your context. But you only actually need one at a time. So rewrite that code to do the minimum you have to do with each thing:

    with open(sys.argv[2]) as f:
        sequence = f.read()
    
    with open(sys.argv[1], newline='') as f:
        db = csv.DictReader(f)
        assert 'name' in db.fieldnames
    
  6. Initialize your counts dictionary on the fly. Or not.

    Once you know the field names, which it turns out are also the STR sequences, you could create an initialized dictionary on the fly.

    str_counts = {}
    for str in db.fieldnames:
        if str != 'name':
            str_counts[str] = 0
    

    This approach uses a dict comprehension to do the same thing:

    str_counts = {str: 0 for str in db.fieldnames if str != 'name'}
    

    It turns out that this is a common need. People frequently need a dictionary initialized with default values. (Gene Rayburn: This thing is sooo commmmonnnn! Audience: How common is it?)

    It's so common that Python has a special type in the standard library just for this case! Presenting: collections.defaultdict

    A defaultdict is a dictionary that knows how to construct a "default value". Because you tell it how! All you need to do is create the dict with a "callable" (a function, lambda, classname, etc.) and it will either look up the key you give it and return the value, or not find the key you give it, call the callable, and return that instead.

    For your purposes, the default value is int, a function which creates a default integer if you call it. And default integers start with a value of 0, so ... #winning!

    # at top of file
    import collections
    
    # ...
    # later in code:
    str_counts = collections.defaultdict(int)
    
  7. Don't guard a regex operation with a regex operation of similar complexity.

    You have a re.findall that is guarded by a re.search. With basically the same pattern, and the same search string, and no other help.

    You could be doing this to save time, but that's a non-starter since search and findall would have the same execution time if the STR does not appear in the sequence.

    You could be doing this to avoid some other operation. But you're not. And it's pretty trivial to write your code in such a way that the "found" path and the "not found" path both produce the same results. Do that instead.

    (Note: there are plenty of times when you do want to put a guard of some kind on an operation. This just isn't one of those times.)

    If max returns 0, and you compute 0 / len(field), you'll be storing 0, which is fine.

  8. Don't delete the names from your database.

    Your comparison starts by deleting the 'name' fields and then using a dict comprehension to copy the other fields to a new dict. Instead of doing that, just use if to exclude the name fields:

    compare = {k: int(v) for k, v in row.items() if k != 'name'}
    
  9. Note that dictionaries can be compared for equality using the == operator:

    if compare == str_counts:
    
  10. A great way to document your code, for problems like these, it to create a "header comment" at the top of the file using Python's triple-quote string mechanism (""" string """) and copy the problem text into it. That gives you something to refer to, and makes the program file entirely self-contained.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback! sys.argv doesn't appear to work in the CS50 IDE I'm using, but I'll keep that in mind for projects outside of CS50. Am I correct in thinking that if I separate the with statements, I should nest them so that both files stay open while I'm using them? Also, on point 7, when I try to remove the outer re.search() I get an error saying max() arg is an empty sequence. Again, maybe this is just an issue with the IDE I'm using, I'm not sure. Still looking through the rest of your suggestions, but wanted to touch on these few things first. \$\endgroup\$ – Star Aug 8 at 20:22
  • \$\begingroup\$ I was able to implement the rest of your suggestions without error, yay! I decided to use dictionary comprehension to initialize my dictionary "on the fly". I also deleted the lines of code that deleted the 'name' fields and instead added an if statement as you suggested. Code is still passing all tests and I appreciate being able to make it shorter. \$\endgroup\$ – Star Aug 8 at 20:34
  • 1
    \$\begingroup\$ Great news. Remember to use sys.argv you have to import sys at the top. You should not need to nest the with statements because all you do with the text file is slurp it into the sequence variable. Just with it, .read() it, and let it die. I don't know why the max arg is an empty sequence. If it works inside the if re.search it should work stand-alone. You'll have to debug that. I don't think there's very many things special about the CS50 environment (which, by the way, thank you for asking this question because I never knew about that project at all -- so freaking cool!) \$\endgroup\$ – Austin Hastings Aug 9 at 4:17

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.