1
\$\begingroup\$

I want to preface this by saying I didn't quite know where to put this because this isn't quite a stack overflow question. So if this is the wrong place to post, please let me know, and please let me know where I could post this type of question.

I recently wrote a program with various functions doing a different thing, however each function was ~100-200 lines long. And since I didn't want to make global parameters, many lines in one function repeated in the other (I redefined the same variables if the user chose to run a different function).

I found my code very difficult to debug, and the general feedback was because it was formatted very poorly, and I should split my code up into multiple functions. However when I tried to look into what should be included in functions, how are they split, etc. It was a bit more vague and unclear.

So I've taken an excerpt from my program and split it into various functions. It gives the proper output as the other program.

The below script basically takes an input (sparta-pred.tab), extracts certain parameters, and modifies it based on user input (seq.txt, mutation_list).

This is the original:

import re


def fun():  
    seq_start=1
    amino_acid_count=(0+seq_start)-1
    sequence_list=[]
    with open('seq.txt') as sequence_file:
        for amino_acid in sequence_file:
            stripped_amino_acid=amino_acid.strip().upper()
            for word in stripped_amino_acid:
                amino_acid_count+=1
                sequence_list.append(str(amino_acid_count)+word)
    y=0
    sparta_file_list1=[]
    sparta_file_list2=[]
    proline_counter=0
    with open('sparta_pred.tab') as sparta_predictions:
        for line in sparta_predictions:
            modifier=line.strip().upper()
            if re.findall('^\d+',modifier):
                A=modifier.split()
                del A[5:8]
                del A[3]
                A[0:3]=["".join(A[0:3])]
                joined=" ".join(A)
                proline_searcher=re.search('\BP',joined)
                if proline_searcher != None:
                    proline_counter+=1
                    if proline_counter<2:
                        proline_count=re.search('^\d+',joined)
                        sparta_file_list1.append(f'{proline_count.group(0)}PN'+' 1000'+' 1000')
                sparta_file_list1.append(joined)
                if proline_searcher != None:
                    y+=1
                    if y==4:
                        proline_count=re.search('^\d+',joined)
                        sparta_file_list1.append(f'{proline_count.group(0)}PHN'+' 1000'+' 1000')
                        y=0
                        proline_counter=0
    mutation_list1=['133R']
    mutation_list2=['133A']
    if mutation_list1==() or mutation_list2==():
        for amino_acids in sparta_file_list1:
            sparta_file_list2.append(amino_acids)
    else:
        for mutations,mutations2 in zip(mutation_list1,mutation_list2):
            for amino_acids in sparta_file_list1:
                if re.findall(mutations,amino_acids):
                    splitting=amino_acids.split()
                    mutation=re.sub(mutations,mutations2,splitting[0])
                    mutation_value=re.sub('\d+.\d+',' 1000',splitting[1])
                    mutation_value2=re.sub('\d+.\d+',' 1000',splitting[2])
                    mutation_replacement=mutation+mutation_value+mutation_value2
                    sparta_file_list2.append(mutation_replacement)
                else:
                    sparta_file_list2.append(amino_acids)
    sparta_file_list3=[]
    for aa in sparta_file_list2:
        modifiers=aa.strip()
        splitter=modifiers.split()
        searcher=re.search('^\d+[A-Z]',splitter[0])
        compiler=re.compile(searcher.group(0))
        sparta_sequence_comparison=list(filter(compiler.match,sequence_list))
        if sparta_sequence_comparison != []:
            sparta_file_list3.append(aa)

    temp_list=[]
    temp_counter=0
    for checker in sparta_file_list3:
        temp_modifier=checker.strip()
        temp_split=temp_modifier.split()
        temp_finder=re.search('^\d+',temp_split[0])
        temp_list.append(temp_finder.group(0))
        temp_counter+=1
        if temp_counter==5:
            if int(temp_finder.group(0))==int(temp_list[0]):
                break
            else:
                del sparta_file_list3[0:4]
                break

    if len(sparta_file_list3)%6 != 0:
        del sparta_file_list3[-5:-1]

That entire thing is under one function in my original program (the same lines are copy/pasted into other functions that use the same files as well). This is my attempt to split the above into functions:

import re

def create_seq_list():
    seq_start=1
    amino_acid_count=(0+seq_start)-1
    sequence_list=[]
    with open('seq.txt') as sequence_file:
        for amino_acid in sequence_file:
            stripped_amino_acid=amino_acid.strip().upper()
            for word in stripped_amino_acid:
                amino_acid_count+=1
                sequence_list.append(str(amino_acid_count)+word)
    return sequence_list
def sparta_formater():
    y=0
    sparta_file_list1=[]
    proline_counter=0
    with open('sparta_pred.tab') as sparta_predictions:
        for line in sparta_predictions:
            modifier=line.strip().upper()
            if re.findall('^\d+',modifier):
                A=modifier.split()
                del A[5:8]
                del A[3]
                A[0:3]=["".join(A[0:3])]
                joined=" ".join(A)
                proline_searcher=re.search('\BP',joined)
                if proline_searcher != None:
                    proline_counter+=1
                    if proline_counter<2:
                        proline_count=re.search('^\d+',joined)
                        sparta_file_list1.append(f'{proline_count.group(0)}PN'+' 1000'+' 1000')
                sparta_file_list1.append(joined)
                if proline_searcher != None:
                    y+=1
                    if y==4:
                        proline_count=re.search('^\d+',joined)
                        sparta_file_list1.append(f'{proline_count.group(0)}PHN'+' 1000'+' 1000')
                        y=0
                        proline_counter=0
    return sparta_file_list1


def mutation_adder():
    sparta_file_list2=[]
    mutation_list1=['133R']
    mutation_list2=['133A']
    if mutation_list1==() or mutation_list2==():
        for amino_acids in sparta_formater():
            sparta_file_list2.append(amino_acids)
    else:
        for mutations,mutations2 in zip(mutation_list1,mutation_list2):
            for amino_acids in sparta_formater():
                if re.findall(mutations,amino_acids):
                    splitting=amino_acids.split()
                    mutation=re.sub(mutations,mutations2,splitting[0])
                    mutation_value=re.sub('\d+.\d+',' 1000',splitting[1])
                    mutation_value2=re.sub('\d+.\d+',' 1000',splitting[2])
                    mutation_replacement=mutation+mutation_value+mutation_value2
                    sparta_file_list2.append(mutation_replacement)
                else:
                    sparta_file_list2.append(amino_acids)
    return sparta_file_list2

def sparta_sequence_filter():
    sparta_file_list3=[]
    sparta_comparison=create_seq_list()
    for aa in mutation_adder():
        modifiers=aa.strip()
        splitter=modifiers.split()
        searcher=re.search('^\d+[A-Z]',splitter[0])
        compiler=re.compile(searcher.group(0))
        sparta_sequence_comparison=list(filter(compiler.match,sparta_comparison))
        if sparta_sequence_comparison != []:
            sparta_file_list3.append(aa)

    return sparta_file_list3


def sparta_bounds_check():
    temp_list=[]
    temp_counter=0
    sparta_filtered_list=sparta_sequence_filter()
    for checker in sparta_filtered_list:
        temp_modifier=checker.strip()
        temp_split=temp_modifier.split()
        temp_finder=re.search('^\d+',temp_split[0])
        temp_list.append(temp_finder.group(0))
        temp_counter+=1
        if temp_counter==5:
            if int(temp_finder.group(0))==int(temp_list[0]):
                break
            else:
                del sparta_filtered_list[0:4]
                break
    if len(sparta_filtered_list)%6 != 0:
        del sparta_filtered_list[-5:-1]

    return sparta_filtered_list

As a minimal example to run the above (if anyone wanted to test it:

#seq.txt
MSYQVLARKW
#sparta_pred.tab
   3    Y   HA     0.000     4.561     4.550     0.018     0.000     0.201
   3    Y    C     0.000   175.913   175.900     0.021     0.000     1.272
   3    Y   CA     0.000    58.110    58.100     0.017     0.000     1.940
   3    Y   CB     0.000    38.467    38.460     0.011     0.000     1.050
   4    Q    N     3.399   123.306   119.800     0.179     0.000     2.598
   4    Q   HA     0.146     4.510     4.340     0.039     0.000     0.237
   4    Q    C    -2.091   173.967   176.000     0.097     0.000     0.914
   4    Q   CA    -0.234    55.623    55.803     0.092     0.000     1.065
   4    Q   CB     3.207    32.000    28.738     0.092     0.000     1.586
   4    Q   HN     0.131     8.504     8.270     0.173     0.000     0.484
   5    V    N     0.131   120.091   119.914     0.078     0.000     2.398
   5    V   HA     0.407     4.575     4.120     0.080     0.000     0.286
   5    V    C     0.162   176.322   176.094     0.109     0.000     1.026
   5    V   CA    -1.507    60.840    62.300     0.078     0.000     0.868
   5    V   CB     0.770    32.625    31.823     0.052     0.000     0.982
   5    V   HN     0.418     8.642     8.190     0.057     0.000     0.443
   6    L    N     7.083   128.385   121.223     0.130     0.000     2.123
   6    L   HA    -0.504     4.085     4.340     0.415     0.000     0.217
   6    L    C     1.827   178.814   176.870     0.195     0.000     1.081
   6    L   CA     3.308    58.271    54.840     0.205     0.000     0.772
   6    L   CB    -1.005    41.051    42.059    -0.005     0.000     0.890
   6    L   HN     0.241     8.694     8.230     0.097    -0.164     0.437
   7    A    N    -4.063   118.812   122.820     0.092     0.000     2.131
   7    A   HA    -0.337     4.023     4.320     0.067     0.000     0.220
   7    A    C     0.433   178.071   177.584     0.090     0.000     1.158
   7    A   CA     2.471    54.552    52.037     0.073     0.000     0.665
   7    A   CB    -0.332    18.690    19.000     0.036     0.000     0.795
   7    A   HN    -0.517     7.889     8.150     0.063    -0.219     0.460
   8    R    N    -4.310   116.247   120.500     0.096     0.000     2.191
   8    R   HA    -0.056     4.313     4.340     0.048     0.000     0.196
   8    R    C     2.152   178.488   176.300     0.060     0.000     0.991
   8    R   CA     1.349    57.485    56.100     0.060     0.000     1.075
   8    R   CB     0.834    31.147    30.300     0.023     0.000     1.040
   8    R   HN     0.244     8.408     8.270     0.109     0.172     0.526
   9    K    N     0.144   120.608   120.400     0.108     0.000     2.283
   9    K   HA    -0.130     4.148     4.320    -0.069     0.000     0.202
   9    K    C     0.691   177.214   176.600    -0.129     0.000     1.048
   9    K   CA     2.415    58.707    56.287     0.008     0.000     0.948
   9    K   CB    -0.114    32.430    32.500     0.074     0.000     0.742
   9    K   HN    -0.617     7.728     8.250     0.159     0.000     0.458
  10    W    N    -4.007   117.283   121.300    -0.016     0.000     2.846
  10    W   HA     0.195     4.850     4.660    -0.009     0.000     0.391
  10    W    C    -1.455   175.056   176.519    -0.013     0.000     1.011
  10    W   CA    -1.148    56.191    57.345    -0.011     0.000     1.832
  10    W   CB     0.166    29.622    29.460    -0.007     0.000     1.151
  10    W   HN    -0.634     7.728     8.180     0.377     0.045     0.582
  11    R    N     1.894   122.475   120.500     0.134     0.000     2.483
  11    R   HA    -0.096     4.293     4.340     0.083     0.000     0.329
  11    R    C    -1.368   174.959   176.300     0.045     0.000     0.961
  11    R   CA    -0.713    55.431    56.100     0.073     0.000     1.041
  11    R   CB     0.187    30.506    30.300     0.033     0.000     0.930
  11    R   HN    -0.880     7.272     8.270     0.107     0.182     0.413
  12    P   HA    -0.173     4.278     4.420     0.051     0.000     0.257
  12    P    C    -1.027   176.281   177.300     0.014     0.000     1.162
  12    P   CA     0.741    63.865    63.100     0.040     0.000     0.762
  12    P   CB     0.046    31.768    31.700     0.036     0.000     0.753
  13    Q    N     1.152   120.951   119.800    -0.001     0.000     2.396
  13    Q   HA     0.193     4.514     4.340    -0.032     0.000     0.220
  13    Q    C     0.275   176.261   176.000    -0.024     0.000     0.900
  13    Q   CA     0.394    56.181    55.803    -0.027     0.000     0.925
  13    Q   CB     2.516    31.223    28.738    -0.051     0.000     1.065
  13    Q   HN     0.012     8.472     8.270     0.002    -0.188     0.535

Both scripts should give identical outputs.

The full code is on github: https://github.com/sam-mahdi/Peaklist_Assignment_Library-PAL-/blob/master/AVS/AVS.py

However, all I am currently looking at in the above is whether I've got the right idea in terms of splitting my code into functions and proper formatting.

Improve this question
\$\endgroup\$
0

3 Answers 3

Reset to default
5
\$\begingroup\$

At first glance, here are the few things I saw that need to be fixed:

  1. Your functions seem pretty long, and this possibly leads to violation of single responsibility principle. Make sure each of them does only a single job.
  2. There are many magic numbers (such as using 0:3, 0:4 while slicing arrays). Same applies to your strings too. As someone who reads your code, I do not understand how/why you used them. Obviously, this is because such usages decrease understandability of your code. Defining constant variables might be needed.
  3. Your variable names are not descriptive enough. Avoid using names such as aa, temp etc.
  4. Name your functions by verbs not by nouns. For example, instead of sparta_formater use format_sparta.
  5. Divide your program into modules. Do not write everything in a single file, separate them according to what they do.
Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ So the functions need to be split even further? Each function is only 1 loop and is doing only one thing. 2. So should I define the slice? I.E. chemical_shifts=A[3] then del chemical_shifts. 3. It was difficult to come up with names prior since I had so much under 1 fun and was running out of names to use, but I can repeat names now that they're split into different functions. 5. Do you mean write separate files and simply import them into my main file loop? \$\endgroup\$
    – samman
    Jun 27, 2020 at 21:12
  • 1
    \$\begingroup\$ Even your functions is only 1 loop, they seem to be doing many things. What I mean by "single responsibility" is that you should have separate functions for parsing the lists or taking inputs etc. By doing so, you can also reduce duplicate code which I believe another problem in your code (for example: proline_count=re.search('^\d+',joined) line is repeated 4 times and that is a smell in code). About your magic numbers (or strings), I recommend to define constant variables. For 5, yes divide your problem into smaller problems and try to solve each in another module (or simply file). \$\endgroup\$
    – Onur Arı
    Jun 29, 2020 at 7:39
  • \$\begingroup\$ Regarding splitting it into different files and importing them as modules. I tried this, but this significantly slowed down my program (10x slower) \$\endgroup\$
    – samman
    Jun 29, 2020 at 13:34
  • 1
    \$\begingroup\$ In normal circumstances, I don't expect importing modules to cause any performance drop. However, if you imported them in a function that you make calls repeatedly, this could be the reason. stackoverflow.com/a/18463100/10054375 This answer might be helpful in this case. \$\endgroup\$
    – Onur Arı
    Jun 29, 2020 at 15:34
  • 1
    \$\begingroup\$ Oh I see. Yes I was calling my function within a loop. I presume redefining the output of my imported value (i.e. some variable=called function), and then looping through that will resolve this (so it doesn't loop through my entire 2nd script). \$\endgroup\$
    – samman
    Jun 29, 2020 at 15:58
3
\$\begingroup\$

I think you are trying to reinvent the wheel. This task can be easily achieved with Pandas. Using Pandas should come with a few benefits:

  • it will make your code more precise,
  • it will reduce its length, and
  • it will increase the readability. Which will make the reviewers give more attention to your code.

After installing Pandas you can use the pandas.read_table function to get going.

Improve this answer
\$\endgroup\$
9
  • \$\begingroup\$ I'v tried doing the exact thing in Pandas before, but came across a variety of shortcomings in what I wanted to achieve (especially when it came trying to extract and modify data in certain ways using series). Additionally, I wanted to write this in pure python, so it doesn't require any dependancies (e.g. pandas) \$\endgroup\$
    – samman
    Jun 28, 2020 at 13:22
  • 1
    \$\begingroup\$ Can you please elaborate on like what problems did you face? \$\endgroup\$
    – Vishesh Mangla
    Jun 28, 2020 at 13:25
  • \$\begingroup\$ This was one such example: stackoverflow.com/questions/58868516/… . However even trying use series I found it difficult to insert values (you may index, but this is designed to work with multiple formats, so the index could always be diff.). NOTE: The script I have posted above (in this post) is a small part of a much larger program. So the link I just posted is to a question of another part of this program (just in case there was any confusion in why they are discussing 2 different things). \$\endgroup\$
    – samman
    Jun 28, 2020 at 13:31
  • \$\begingroup\$ Sorry, I 'm not used to understand such big posts. I 'm loading your file in my jupyter notebook. Can you guide me through the steps so that I can see the error? \$\endgroup\$
    – Vishesh Mangla
    Jun 28, 2020 at 13:42
  • 1
    \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$
    – samman
    Jun 28, 2020 at 14:04
2
\$\begingroup\$

Function naming

As @OnurArı suggested, your naming needs work. In particular, this:

def fun():

is probably worse than having no function at all, so it's good that you attempted to split it up.

Some nuance to the other answer: yes, mutation_adder should be add_mutation (imperative-tense verb) as a function. Where you would see MutationAdder (noun) is if it's a class name.

List formation

mutation_list1=['133R']
mutation_list2=['133A']
if mutation_list1==() or mutation_list2==():
    for amino_acids in sparta_formater():
        sparta_file_list2.append(amino_acids)

doesn't make a huge amount of sense. Neither of those conditions will ever be true, because you just populated the lists manually. Even if the lists "might" be empty, you should be comparing them to empty lists [] instead of empty tuples ().

Further, you don't need that for. Since sparta_formater returns an iterable, you could simply

sparta_file_list2.extend(sparta_formater())

Finally, don't call variables list1 and list2. The numbers hide the actual intent of the variable, and it's not really even useful to call something a list - it's more informative to add a type hint and pluralize the variable name, i.e.

mutations: List[str] = ['133R']

Regexes

This:

    compiler=re.compile(searcher.group(0))

is not a compiler. re.compile, the function, is technically the compiler, but what you get out of it is a regular expression object. Don't call compile within a loop - the whole purpose that compile exists is to pay the cost of compilation once up front, outside of the loop, so that using it within the loop is faster.

Improve this answer
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Thank you for the feedback. Only one thing to comment, for mutation_list, this is only an excerpt of a much larger script that uses GUI inputs. In some cases mutation_list will be empty, in others it won't. However, when splitting them up, I had to redefine mutation_list so it would work in my test case. That's why I have that if mutation_list is empty (because it might be in some cases) \$\endgroup\$
    – samman
    Jun 29, 2020 at 16:18
  • 1
    \$\begingroup\$ That's fine, though - when eliding code for the purposes of a review, it's very important to show something like # elided for review brevity to make it more obvious where things are missing. \$\endgroup\$
    – Reinderien
    Jun 29, 2020 at 16:25
  • 1
    \$\begingroup\$ Yeah, thank you! This is my first time writing a biggerish program, and while I think I have a decent grasp now on writing code (at least the basics), I am completely unfamiliar with, I believe it might be software design? Basically I can write the script, but I have no idea how to format it or structure it so it isn't just one long massive file. Thus, trying to learn how to use functions, naming terminology, learning how to comment, etc. \$\endgroup\$
    – samman
    Jun 29, 2020 at 16:33
  • 1
    \$\begingroup\$ You're in the right place :) \$\endgroup\$
    – Reinderien
    Jun 29, 2020 at 16:34
  • \$\begingroup\$ @Reinderen Can their code be improved by my deleted post? codereview.stackexchange.com/a/244694/226167 . \$\endgroup\$
    – Vishesh Mangla
    Jun 30, 2020 at 8:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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