Sign up ×
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:


User mutation input file:


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
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)
    for j in i:
    for index,letter in c.items():
    print temp_string


combination :E2R
combination :C4W
combination :E6G
combination :E2R_C4W
combination :E2R_E6G
combination :C4W_E6G
combination :E2R_C4W_E6G

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:

    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 '14 at 9:34
@Nobody yes change should be lower case. to know the change –  sundar nataraj Jul 18 '14 at 9:38

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:


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:

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 '14 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 '14 at 10:06
+1 for clear succinct explanations rather than just changes. –  trichoplax Jul 21 '14 at 9:46

Your Answer


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.