Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have recently answered one question on Stack Overflow.

Problem statement:

Input sequence:

PEACCEL

User mutation input file:

E2R C4W E6G

The algorithm is for an input sequence of, say, PEACCEL and mutation E2R. We need to replace second character in sequence, that is, E to R, and so on for the combinations of mutations.

I provided this answer:

from itertools import combinations,chain
from collections import Counter
input_sequence='PEACCEL'
mutations=map(lambda x:x.strip(),open('a.txt','r').readlines())
 #['E2R', 'C4W', 'E6G']
mutation_combinations= chain.from_iterable([list(combinations(mutations,i))for i in range(1,4)])
 #[('E2R',), ('C4W',), ('E6G',), ('E2R', 'C4W'), ('E2R', 'E6G'), ('C4W', 'E6G'), ('E2R', 'C4W', 'E6G')]

for i in mutation_combinations:
    print "combination :"+'_'.join(i)
    c=Counter({})
    temp_string=input_sequence
    for j in i:
        c[j[1]]=j[2].lower()
    for index,letter in c.items():
        temp_string=temp_string[:int(index)-1]+letter+temp_string[int(index):]
    print temp_string

Output:

combination :E2R
PrACCEL
combination :C4W
PEAwCEL
combination :E6G
PEACCgL
combination :E2R_C4W
PrAwCEL
combination :E2R_E6G
PrACCgL
combination :C4W_E6G
PEAwCgL
combination :E2R_C4W_E6G
PrAwCgL

Algorithm I followed:

  1. Read the mutation sequences like E2R.... from a file using

    mutations=map(lambda x:x.strip(),open('a.txt','r').readlines())
    
  2. Made the combinations of the mutations

    mutation_combinations= chain.from_iterable([list(combinations(mutations,i))for i in range(1,4)])
    

    If you have 4 mutations you want all four change the range value to 5.

  3. For each combination I replaced them with specified character:

    for j in i:
        c[j[1]]=j[2].lower()
    

    I used the above counter to keep track of which character was to be replaced during the mutation combination.

How can I make this more efficient? I also need to know of any valuable pointers I should know while coding.

share|improve this question
    
Is the change of case intended? E.g: C4W_E6G creates PEAwCgL instead of PEAWCGL. –  Nobody Jul 18 at 9:34
    
@Nobody yes change should be lower case. to know the change –  sundar nataraj Сундар Jul 18 at 9:38
add comment

1 Answer 1

There are several things that I would change in your code:

Use functions

This is a good example of spaghetti code where everything is meshed together. You should provide functions to load the mutation list, one to execute one mutation and one that executes a list of mutations.

Printing the results should also be an extra function.

Use a better mutation representation

Your internal representation for the replacements is too leaky (and maybe too easy).

What if the requirements change and you want to replace multiple character strings?

Instead you should use a structure that already does the separation of the search term, the counter and replace term.

Another problem here is that you will not be able to pass replacement indices bigger than 9 because you only take one character instead of matching a regex for the number in the middle and taking the whole number as index.

Missing error handling

Talking of indices, your program will fail when the given index is out of bounds for the input string.

Unclear input

Neither you nor the OP on SO state what the first element in the mutation string is for. I don't see it being used. It seems to duplicate some of the information of the replacement index.

Code formatting

Please improve the code formatting. Take for example this:

temp_string=temp_string[:int(index)-1]+letter+temp_string[int(index):]

I had a hard time finding the boundaries of the [] because everything is so close together. Instead try to leave space around operators/operands:

temp_string = temp_string[:int(index) - 1] + letter + temp_string[int(index):]

Misused collection

I don't see why you are using a Counter in your code. You don't seem to count anything with it and use it like an ordinary dict.

The line

for j in i:
        c[j[1]]=j[2].lower()

Looks dubious, what is it for? If there are several mutation strings with the same offset only the last one will be recorded (because it overwrites the previous ones). Is this the intended behavior?

If you do not want this behavior then why use the dict (or rather Counter) at all? You could have iterated through the mutations and directly applied them with the same result.

share|improve this answer
    
the first letter in the string is for if you see the input sequance we are replacing the E at 2 index to r :) –  sundar nataraj Сундар Jul 18 at 10:04
    
Nobody you have given me valuable . thank you man ! surely i will keep that in mind +1 :). and going through your answer two to three times so that my mind keep all things remebered –  sundar nataraj Сундар Jul 18 at 10:06
    
+1 for clear succinct explanations rather than just changes. –  githubphagocyte yesterday
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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