Words dictionary out of a text file

Question

Please tell me what the weak spots of this code are (specially in terms of efficiency) and how I can improve it:

def get_word_frequencies(filename):
  handle = open(filename,'rU')
  text = handle.read()
  handle.close()
  MUST_STRIP_PUNCTUATION = ['\n','&','-','"','\'',':',',','.','?','!'\
    ,';',')','(','[',']','{','}','*','#','@','~','`','\\','|','/','_'\
    ,'+','=','<','>','1','2','3','4','5','6','7','8','9','0']
  text = text.lower()
  for char in MUST_STRIP_PUNCTUATION:
    if char in text:
      text = text.replace(char,' ')
  words_list = text.split(' ')
  words_dict = {}
  for word in words_list:
    words_dict[word] = 0
  for word in words_list:
    words_dict[word] += 1
  del words_dict['']
  return words_dict

Some steps sound repetitive to me and it seems that I'm looping on the text many times but I think I'm obliged to take each of those steps separately(unless I'm wrong), for instance, replacing invalid characters should be on multiple separate iterations, or lower casing the whole text must be a separate step, and so on.

Also for creating the dictionary I'm suspicious a way better than words_dict[word] = 0 must exist?

Answer 1

Some in-place improvements you could make:

• PEP-8 recommends four space indentation, not two;

• collections.defaultdict would allow you to scan the list of words only once, without having to explicitly initialise to 0 for each word. Even easier, there's a collections.Counter, too;

• Rather than iterate over all of the characters you want to replace, you could use re.sub with a regular expression pattern matching those characters (or re.findall for the characters you do want); and

• You should generally use the with "context manager" to handle files; see e.g. the tutorial.

More broadly, though, you have one function with three different purposes:

1. Handling the file;

2. Cleaning up the text; and

3. Counting the words.

Splitting these separate concerns up into separate functions would make those functions easier to test, understand and reuse.

With these ideas applied, you could write something like:

from collections import Counter
import re

def get_text(filename):
    """Read and return the content of a specified file."""
    with open(filename, "rU") as handle:
         return handle.read()

def clean_text(text):
    """Lowercase text and extract words."""
    return re.findall("[a-z]+", text.lower())

def get_word_frequencies(filename):
    text = get_text(filename)
    words = clean_text(text)
    return Counter(words)

Answer 2

General Implementation Suggestions

1. First of all I would suggest that the function is doing too much - and could be broken into multiple smaller functions to keep single behaviour in a single function. This is a good mindset to get into for unit testing and mocking (later down the line).

   For example currently your method is doing the following 2 bit of behaviour:

   1. Opening, Reading and Closing a file from the file system.
   2. Collecting the word frequency

   I would suggest having 2 methods:

   def open_and_read_file_content(filepath):
     handle = open(filename,'rU')
     text = handle.read()
     handle.close()
     return text
   
   def collect_word_frequency_in_text(text):
     ...
   
   

Code Review

First I would suggest that there is a redundant if statement when checking if the punctuation character is in the text body and you just want to call the replace.

In this case if the punctuation doesn't exist you are doing the same number of iterations as currently, however, if it does exist it is replaced there and then rather than being checked before hand:

# Before
...
for char in MUST_STRIP_PUNCTUATION:
    if char in text:
      text = text.replace(char,' ')
...

# After
...
for char in MUST_STRIP_PUNCTUATION:
    text = text.replace(char,' ')
...

Finally, while the 2 for statements don't look pretty and look like they can be made clearer - if efficiency is what you are looking for - I am not sure of any solution that will be better.

Currently the time complexity (for the two loops) is O(2n) - you could collect each unique entry in the text block and then count each occurrence of the word using Python's count(), however I am not sure this will be more time efficient but might look cleaner.

Additionally - You could do an if to check the word != '' rather than deleting it from the set at the end of the process - however, depending on the frequency of '' in the data set, that might not be desired.

I would stress though, that similar to my initial point made before the code review section, for readability and understanding (debugging/testing) this might be desired (example below)

...
  words_list = text.split(' ')
  words_dict = {}
  for word in words_list:
    if(word != '')
      words_dict[word] = text.count(word)
  return words_dict
...