I would like to hear feedback from an interviewer's perspective on how to improve the following code:

from collections import Counter

def word_map(string):
    word_dict = {}
    stop_word_file=open("stopwords.txt", "r")
    stop_words =stop_word_file.read().split()
    for word in string.split():
        word = filter(str.isalnum, word).lower()
        word = word.strip()
        if word != '' and word not in stop_words:
            if word in word_dict.keys():
                word_dict[word] +=1
                word_dict[word] = 1

    return word_dict

file = open("story.txt", "r")

five_top_words = Counter(word_map(file.read())).most_common(5)
for letter, count in five_top_words:
    print '%s : %10d' %(letter, count)
  • \$\begingroup\$ What is the purpose of 'stopwords.txt' exactly, what would be a typical content? \$\endgroup\$ May 31 '16 at 19:46
  • \$\begingroup\$ @MathiasEttinger wget https://www.lexalytics.com/files/stopwords.txt \$\endgroup\$
    – Mona Jalal
    May 31 '16 at 20:29
  • 1
    \$\begingroup\$ With a bit of work your tokenizer could be broken by corner cases. "the interviewer's opinion" vs "the three interviewers" -- those are different words (interviewer's vs interviewers) but you are counting them the same. The solutions would be to use a proper tokenizer like from NLTK. Would you like me to extend this into an answer? \$\endgroup\$ Jun 1 '16 at 2:54
  • 5
    \$\begingroup\$ Now I want to know which words appear most frequently in Python code. \$\endgroup\$
    – user253751
    Jun 1 '16 at 6:33
  • 1
    \$\begingroup\$ The not is because it is work for me, which would be wasted if you wanted a stand-alone implementation, or didn't care about corner cases. Some more also "its" vs "it's", and "we'll" vs "well", and "I'll" vs "ill". Another possibility is the traditional tokenizing of things like "isn't" as "is", and "n't", which may or may not be desirable. I'll write an answer up later (poke me if I don't) \$\endgroup\$ Jun 1 '16 at 7:58

I second @Dannnno arguments except for the fact that you don't need to split the work into tinier functions.

In fact I'd say quite the opposite as using more function let you turn them into generators for a more efficient memory usage.

You should also make yourself familiar with the str.format syntax which is the new way to go over the % formatting syntax.

My proposal is:

from collections import Counter
from itertools import chain

def word_map(filename, stop_words_file):
    with open(stop_words_file) as f:
        stop_words = set(f.read().split())

    with open(filename) as f:
        for line in f:
            words = (filter(str.isalnum, w).lower() for w in line.split())
            yield (w for w in words if w and w not in stop_words)

def top_words(filename, stopfile, count):
    words = chain.from_iterable(word_map(filename, stopfile))
    return Counter(words).most_common(count)

if __name__ == '__main__':
    for word, count in top_words('story.txt', 'stopwords.txt', 5):
        print '{} : {:>10}'.format(word, count)

The first thing you should do is make this a "proper" script - functions, and an if __name__ == '__main__' block. This allows it to be executed as a standalone script, or imported as needed.

Next, you need to make sure to close your files. The easiest way to do this is with context managers.

Next you'll want to make it a but more generalized - pass more things as parameters instead of hardcoding them. This includes filenames, the number of words, etc.

Next, store your stop_words in a frozenset for more efficient lookup.

Next, use the Counter more effectively by using it within the word_map function and using a list comprehension as well. Write a function to filter for readability.

Lastly, use better names and combine your two helper functions - they're simple enough that you don't need both of them.

from collections import Counter

def find_n_top_words(source_filename, stopwords_filename, num_top_words):
    with open(source_filename, 'r') as source_file, \
         open(stopwords_filename, 'r') as stop_word_file:

        stop_words = frozenset(stop_word_file.read().split())
        words = [filter(word.isalnum).lower().strip() for word in string.split()]

        def filter_func(word):
            return word and word not in stopwords

        filtered_words = filter(filter_func, words)
        word_dict = Counter(filtered_Words)
        top_words = word_dict.most_common(num_top_words)
        for word, count in top_words:
            print '%s : %10d' % (word, count)

if __name__ == '__main__':
    find_n_top_words('story.txt', 'stopwords.txt', 5)

I'd suggest these changes:

  1. Use a frozenset for the stop words.
  2. Create a cached global variable for the stop words and filespec. Use the filespec to determine when to load/reload.
  3. Accept an optional named parameter for the stopwords file. If no value is given, use a default value (or die).
  4. Create the Counter in your function.
  5. Either return the Counter, or return the Counter's .most_common words, as you like.

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.