Skip to main content
added 1 character in body
Source Link
Gareth Rees
  • 49.7k
  • 3
  • 129
  • 210
    for anagrams in anagram_dict.values():
        for word1, word2 in combinationcombinations(anagrams, 2)
    for anagrams in anagram_dict.values():
        for word1, word2 in combination(anagrams, 2)
    for anagrams in anagram_dict.values():
        for word1, word2 in combinations(anagrams, 2)
Source Link
Gareth Rees
  • 49.7k
  • 3
  • 129
  • 210

  1. The code is well-documented and clear.

  2. The docstring for make_words_list says

     returns a list of words in that file.
    

but if you look carefully at the behaviour, what it does it return a list containing the first word on each line of the file:

    word = line.split()[0]
  1. When you have a function like make_words_list that returns a list of results, it's often more convenient to generate the results one at a time using the yield statement. This means that if the results are also being consumed one at a time, then the whole list does not have to be kept in memory.

So make_words_list could become:

    def read_words(filename):
        "Generate the first word on each line of the named file."
        with open(filename) as file:
            for line in file:
                yield line.split()[0]

If the caller really does need a list, it is easy enough to call list:

    list(read_words(filename))
  1. anagram_sets could be simplified using collections.defaultdict, like this:

     anagram_dict = defaultdict(list)
     for word in wordlist:
         anagram_dict[sorted_word(word)].append(word)
    
  2. But actually it might be even better to use sets here rather than lists — this would ensure that no word could get added twice, and so you could avoid the test word1 != word2 later on.

     anagram_dict = defaultdict(set)
     for word in wordlist:
         anagram_dict[sorted_word(word)].add(word)
    
  3. In anagram_dict you go to some trouble to remove words with no anagrams. But this is unnecessary — if you look at the logic in find_metathesis_pairs you'll see that nothing will go wrong if len(anagram_dict[word_family]) is 1. So it would be simpler to leave the dictionary alone.

  4. is_metathesis_pair is missing a docstring.

  5. is_metathesis_pair returns the string "Error" to indicate an error. It is risky to return an exceptional value like this to indicate an error, because the caller may forget to check for the exceptional value. And in fact this is what happened:

     if is_metathesis_pair(word1, word2):
    

So in the error case, is_metathesis_pair returns the string "Error", but this treated the same as True by the if statement, and so the error gets ignored.

It is better to raise an exception in exceptional cases.

  1. is_metathesis_pair maintains a running count of mismatched letters in order to provide an early exit from the loop. But in practice we expect words to be short: in my computer's dictionary the average word length is just 10 letters. So we are not saving very much time by exiting early from the loop, and we are paying the cost of checking the count.

So I think it would be better not to worry about the early exit, and just count all the mismatches:

    count = sum(l1 != l1 for l1, l2 in zip(word1, word2))
  1. In find_metathesis_pairs there is a loop over the keys of a dictionary:

     for word_family in anagram_dict.keys():
    

and then in the body of the loop the expression anagram_dict[word_family] occurs several times. When you have a loop like this it is better to loop over the keys and values simultaneous:

    for word_family, anagrams in anagram_dict.items():

But if you do this, you'll see that you don't actually need word_family any more: it was only ever needed to look up the list of anagrams. So you might as well only loop over the values:

    for anagrams in anagram_dict.values():
  1. There is a loop over all pairs of distinct words in each family:

     for word_family in anagram_dict.keys():
         for word1_index in range(len(anagram_dict[word_family])):
             word1 = anagram_dict[word_family][word1_index]
             for word2 in anagram_dict[word_family][word1_index+1:]:
    

When you have a loop over distinct pairs, you can use itertools.combinations to combine the two loops into one:

    for anagrams in anagram_dict.values():
        for word1, word2 in combination(anagrams, 2)
  1. The changes I've suggested above have reduced the length of the code considerably. It might now make sense to inline some of the functions at their point of use. This results in the following:

     from collections import defaultdict
     from itertools import combinations
    
     def find_metathesis_pairs(words):
         "Generate the metathesis pairs in an iterable of words."
         families = defaultdict(set)
         for word in words:
             families[''.join(sorted(word))].add(word)
         for family in families.values():
             for pair in combinations(family, 2):
                 if sum(l1 != l2 for l1, l2 in zip(*pair)) == 2:
                     yield pair
    

Inlining functions doesn't always make sense, and you might legitimately prefer the version of the code with multiple small functions, but in this case I think joining them together makes the logic easier to follow.