5
\$\begingroup\$

I've written a piece of code which combines given letters in groups of n letters, and writes all the combinations in a text file. This is my first attempt and I wonder if there is an optimal way to do this.

letter_input = raw_input(' Type in letters: ')

number_input = int(raw_input(' Letters per word: '))

split_letters = list(letter_input)


def function():  

    all_combinations_list = list(itertools.permutations(split_letters, number_input))

    output_file = open('/tmp/list.txt', 'w')

    number_of_combinations = len(all_combinations_list)

    for n in range(0,number_of_combinations):
        raw_characters = str(all_combinations_list[n])
        out_1 = raw_characters.translate(string.maketrans('',''), string.punctuation)
        out_2 = out_1.replace(' ','')
        print >>output_file, out_2


    output_file.close() 
\$\endgroup\$
7
\$\begingroup\$

Don't use global variable

It make things hard to understand and to maintain. In your case, the simple way to do this is to feed your function function() the parameters it needs.

def function(letter_input, number_input, split_letters):

Make your code easier to read

You can ensure you respect PEP 8, the guideline to write Python code. You'll find various tools to help you comply to the usual standards : pep8, pylint, pyflakes, pychecker.

Also, function is a pretty bad function name, let's pick something better (for instance write_permutation).

Use pythonic loops

Python provides you a clean/safe/concise/way to iterate over pretty much anything : for something in something_iterable. Using the loop index is much more verbose on top of being less efficient.

You can change :

all_combinations_list = list(itertools.permutations(split_letters, number_input))
number_of_combinations = len(all_combinations_list)
for n in range(0,number_of_combinations):
    raw_characters = str(all_combinations_list[n])

to (using the default behavior for range)

all_combinations_list = list(itertools.permutations(split_letters, number_input))
number_of_combinations = len(all_combinations_list)
for n in range(number_of_combinations):
    raw_characters = str(all_combinations_list[n])

then

all_combinations_list = list(itertools.permutations(split_letters, number_input))
for e in all_combinations_list:
    raw_characters = str(e)

then

for e in list(itertools.permutations(split_letters, number_input)):
    raw_characters = str(e)

then (because you don't need to create a temporary list) :

for e in itertools.permutations(split_letters, number_input):
    raw_characters = str(e)

Don't use complicated non-required logic

Converting your tuple in a string you perform logic in in way too complicated on top of not being efficient. You could just do something like (using what we call "tuple unpacking"):

for e in itertools.permutations(split_letters, number_input):
    a, b, c = e
    out_2 = a+b+c

This can even be more concise by doing the unpacking as part of the loop :

for a, b, c in itertools.permutations(split_letters, number_input):
    print >>output_file, a+b+c

Using with

You can use the with keyword to handle ressources that needs to be "opened" and "closed" such as files. In your case, you can write :

with open('/tmp/list.txt', 'w') as f:
    for a, b, c in itertools.permutations(split_letters, number_input):
        print >>f, a+b+c

and you don't need to close the file manually.

Using the if__name__ guard

It is usually a good habit to put your code behind a guard to have your main code running only if we run your script and not if we import it (because one wants to re-use a function for instance).

No need for list as strings are iterables too

As pointed out in codesparkle's comment, you don't need to build a list.

Once this is done, you code looks like :

import itertools

def main():
    """Main function"""
    split_letters = raw_input(' Type in letters: ')
    number_input = int(raw_input(' Letters per word: '))
    with open('/tmp/list.txt', 'w') as f:
        for a, b, c in itertools.permutations(split_letters, number_input):
            print >>f, a+b+c


if __name__ == "__main__":
    main()

As a final note, your code could be written using a single call to print by using join on the list of permutations but this doesn't bring much so I'll skip that part.

\$\endgroup\$

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.