Finding words that rhyme

Question

Preface

I was trying to review this question on the same topic, but in the end many points I wanted to make were excellently explained by @ferada so I felt that posting my code and explaining the concepts around such changes would just be repetition.

Instead I am questioning whether my approach is over-engineered considering the simplicity of this task.

---

Specification

I consider two words to rhyme if they share a certain number of phonemes at the end.

The division of the words into phonemes is already given from input and must only be read into program data.

Data is fetched from the internet on the first run and stored in a local file for successive runs to save internet traffic and execution time.

---

Code

import doctest
import os
import re
import requests
from collections import namedtuple

URL = "http://svn.code.sf.net/p/cmusphinx/code/trunk/cmudict/cmudict-0.7b"
LIBRARY = "library.txt"
PHONEMES_TO_MATCH = 3

ReadingData = namedtuple("ReadingData", ["word", "phonemes"])

def first_and_others(xs):
    """
    >>> first_and_others([1, 2, 3])
    (1, [2, 3])
    """
    return xs[0], xs[1:]

def format_data(raw_data):
    '''
    Transforms a list of lines of the format
      WORD [SPACE] PHONEME1 [SPACE] PHONEME2 [SPACE] PHONEME3 ...
    into a list of `ReadingData` classes for easier further processing.

    >>> format_data(["ZURCHER Z ER1 K ER0", "ZUREK Z UH1 R EH0 K"])
    [ReadingData(word='ZURCHER', phonemes=['Z', 'ER1', 'K', 'ER0']), ReadingData(word='ZUREK', phonemes=['Z', 'UH1', 'R', 'EH0', 'K'])]
    '''
    return [ReadingData(*first_and_others(x.strip().replace('\n','').split()))\
             for x in raw_data \
              if not x.startswith(';;;')]

def write_url_to_filename(url, filename):
    response = requests.get(URL, stream=True)

    if not response.ok:
        raise Exception("Error writing {} to {}".format(url, filename))

    with open(filename, 'wb+') as f:
        for block in response.iter_content(4096):
            f.write(block)

def find(predicate, iterable):
    """
    >>> find(lambda x: x % 2 == 0, [1, 2, 3, 4])
    2
    """
    return next(i for i in iterable if predicate(i))

def last_items_match(a, b, how_many):
    """
    >>> last_items_match([1, 2, 3], [7, 2, 3], 2)
    True
    """
    return a[-how_many:] == b[-how_many:]

def find_rhymes(word, data, how_many_phonemes=2):
    """
    Finds all the words in `data` that rhyme with `word`, that is,
    all the words that end in the same `how_many_phonemes` phonemes.

    `word` itself is always included as each word rhymes with itself
    (except when it has less than `how_many_phonemes` phonemes).

    >>> find_rhymes("example", [ReadingData('EXAMPLE', ['IH0', 'G', 'Z', 'AE1', 'M', 'P', 'AH0', 'L']),
    ...                         ReadingData('AMPAL', ['AE1', 'M', 'P', 'AH0', 'L']),
    ...                         ReadingData('HAPPINESS', ['HH', 'AE1', 'P', 'IY0', 'N', 'AH0', 'S'])])
    [ReadingData(word='EXAMPLE', phonemes=['IH0', 'G', 'Z', 'AE1', 'M', 'P', 'AH0', 'L']), ReadingData(word='AMPAL', phonemes=['AE1', 'M', 'P', 'AH0', 'L'])]

    """
    this_phonemes = find(lambda line: line.word == word.upper(), data).phonemes
    return [candidate for candidate in data \
             if last_items_match(candidate.phonemes, this_phonemes, how_many_phonemes)]

def main():
    if not os.path.isfile(LIBRARY):
        write_url_to_filename(URL, LIBRARY)
    with open(LIBRARY) as f:
        data = format_data(f.readlines())
    print(find_rhymes("FORCE", data, PHONEMES_TO_MATCH)[:20])

if __name__ == "__main__":
    doctest.testmod()
    main()

Answer 1

Your use of constants is a bit buggy. First of, since you call write_url_to_filename(URL, LIBRARY), you should change

def write_url_to_filename(url, filename):
    response = requests.get(URL, stream=True)

into

def write_url_to_filename(url, filename):
    response = requests.get(url, stream=True)

i.e: use the parameter instead of the constant.

Secondly, since you only use your constants in main and pass them as parameters afterwards, I feel it would be best to have them as parameters with default value instead of constants:

def main(library='library.txt',
         url='http://svn.code.sf.net/p/cmusphinx/code/trunk/cmudict/cmudict-0.7b',
         phonemes=3):
    if not os.path.isfile(library):
        write_url_to_filename(url, library)
    with open(library) as f:
        data = format_data(f.readlines())
    print(find_rhymes("FORCE", data, phonemes)[:20])

I would also have named write_url_to_filename simply download_into but it's not guaranteed that it is a better name.

---

On a stylistic note, you make extensive use of \ in your list-comprehensions where implicit line continuation are at play. You can drop them, it won't change the meaning.

You also seem to indent them one extra space after each line, try to avoid that.

---

find is just applying a filter and taking the first element. Since you are using Python 2, you may be insterested in itertools.ifilter instead of filter. The former being an iterator whereas the latter returns a list.

Not that your approach is bad per se, but you might want to compare the two, performance wise:

from itertools import ifilter

def find(predicate, iterable):
    return next(ifilter(predicate, iterable))

---

When building your ReadingData, your x.strip().replace('\n','').split()) can be replaced by x.split() as split without arguments has extra logic to clean-up a string while performing the split.

I would also change your helper function into:

def clean_line(line):
    data = line.split()
    return ReadingData(data[0], data[1:])

so that:

1. you can simplify format_data:

   def format_data(raw_data):
       return [clean_line(x) for x in raw_data if not x.startswith(';;;')]
   

2. you let the possibility to the writer of the question that led to this code to write, in Python 3:

   def clean_line(line):
       word, *phonemes = line.split()
       return ReadingData(word, phonemes)
   

   which is even more explicit

3. You can use it directly in your with block without having to read the whole file into memory first:

   def main(library='library.txt',
            url='http://svn.code.sf.net/p/cmusphinx/code/trunk/cmudict/cmudict-0.7b',
            phonemes=3):
       if not os.path.isfile(library):
           write_url_to_filename(url, library)
       with open(library) as f:
           data = [clean_line(line) for line in f if not line.startswith(';;;')]
       print(find_rhymes("FORCE", data, phonemes)[:20])
   

   Alternatively, you could just pass the file f to format_data instead of the list of lines. It will work without modifications and will also let you build the data as you read the file instead of reading the file first and then building data.

Answer 2

Firstly there are some minor stylistic errors that can be detected with pylint. Most notably you never use the re module.

I looked mostly at this section of code:

def first_and_others(xs):
    """
    >>> first_and_others([1, 2, 3])
    (1, [2, 3])
    """
    return xs[0], xs[1:]

def format_data(raw_data):
    '''
    Transforms a list of lines of the format
      WORD [SPACE] PHONEME1 [SPACE] PHONEME2 [SPACE] PHONEME3 ...
    into a list of `ReadingData` classes for easier further processing.

    >>> format_data(["ZURCHER Z ER1 K ER0", "ZUREK Z UH1 R EH0 K"])
    [ReadingData(word='ZURCHER', phonemes=['Z', 'ER1', 'K', 'ER0']), ReadingData(word='ZUREK', phonemes=['Z', 'UH1', 'R', 'EH0', 'K'])]
    '''
    return [ReadingData(*first_and_others(x.strip().replace('\n','').split()))\
             for x in raw_data \
              if not x.startswith(';;;')]

Your list comprehension seems relatively long to me and I had trouble understanding exactly what it does:

[ReadingData(*first_and_others(x.strip().replace('\n','').split()))\
             for x in raw_data \
              if not x.startswith(';;;')]

I considered a way of breaking it up:

# Explain why we don't want ;;;
thesaurus = filter(lambda line: not line.startswith(';;;'), raw_data)
# Explain why you do this too... You could also use a list comprehension...
thesaurus = map(lambda x: x.strip().replace('\n', '').split(), thesaurus)
# Separate into WORD : [PHENOME]
thesaurus = [(line[0], line[1:]) for line in thesaurus]
return [ReadingData(*entry) for entry in thesaurus]

This way we also don't need first_and_others which you use only once, and is longer than xs[0], xs[1:].

There may be some performance penalties, but it seems to run fairly well on the example you gave when I run it.