5
\$\begingroup\$
# Program will display a welcome message to the user
print("Welcome! This program will analyze your file to provide a word count, the top 30 words and remove the following stopwords.")

s = open('Obama 2009.txt','r').read()  # Open the input file

# Program will count the characters in text file
num_chars = len(s)

# Program will count the lines in the text file
num_lines = s.count('\n')

# Program will call split with no arguments
words = s.split()
d = {}
for w in words:
    if w in d:
        d[w] += 1
    else:
        d[w] = 1

num_words = sum(d[w] for w in d)

lst = [(d[w],w) for w in d]
lst.sort()
lst.reverse()

# Program assumes user has downloaded an imported stopwords from NLTK
from nltk.corpus import stopwords # Import the stop word list
from nltk.tokenize import wordpunct_tokenize

stop_words = set(stopwords.words('english')) # creating a set makes the searching faster
print ([word for word in lst if word not in stop_words])

# Program will print the results
print('Your input file has characters = '+str(num_chars))
print('Your input file has lines = '+str(num_lines))
print('Your input file has the following words = '+str(num_words))

print('\n The 30 most frequent words are /n')

i = 1
for count, word in lst[:50]:
    print('%2s. %4s %s' %(i,count,word))
    i+= 1


print("Thank You! Goodbye.")
\$\endgroup\$
  • \$\begingroup\$ What should I do with this code ? \$\endgroup\$ – Grajdeanu Alex. Apr 23 '16 at 16:03
  • 2
    \$\begingroup\$ @Dex'ter review it. It's even documented. \$\endgroup\$ – ferada Apr 23 '16 at 16:11
  • 2
    \$\begingroup\$ It's generally helpful if you include a problem statement about what the program is supposed to do. Then we can compare what the program is supposed to do to what it actually does. As it is, we can only compare what it says it's doing to what it actually does. \$\endgroup\$ – mdfst13 Apr 23 '16 at 16:27
  • \$\begingroup\$ Hi, the problem is that the results of the top 30 words that I want to pull from the file return with the stop words in the list. My question is how do I return the top 30 without the stop words ("a", "the", "if", "is", "to")? \$\endgroup\$ – Karla Richardson Apr 24 '16 at 20:06
  • \$\begingroup\$ Do you have a link to Obama 2009.txt \$\endgroup\$ – Martin York Apr 25 '16 at 20:39
3
\$\begingroup\$

The code is clear, if a bit overly commented. All in all very good I'd say. Once you do become more familiar with it, things like len(...) should be obvious enough that they generally don't warrant a comment on their own.

Also in general, single character names are discouraged as they don't carry much meaning and can be harder to read. On the top of my head s is vaguely a string, d could be a dictionary ... but really, what string and what dictionary I have no idea.

Next, while it works, the general recommendation in longer scripts is not to use open directly without with, unless you take care to close the file. Therefore the input file should be read like follows:

# Open the input file
with open('Obama 2009.txt', 'r') as f:
    s = f.read()

The with statement will take care to always call close on the open file, which would otherwise lead to issues in long-running processes taking up too much system resources (open files).

For the word counting I'd again say it's fine, but can be written more compact. Notably using a class from collections, Counter.

num_words on the other hand does too much work and can just be len(words).

list.sort actually has a reverse parameter, so you can just use that instead of the extra call:

lst.sort(reverse=True)

Usually imports go to the start of the file, even though it doesn't affect function too much (but you'd get an early warning if a library wasn't available for example).

For formatting output there are a ton of options. At some point you might want to look at the format method for strings.

Lastly, the loop at the end prints the 50 most frequent words, not 30 like the output suggests. That is a good opportunity to introduce a constant for the number of words to print:

PRINT_WORDS = 50

print('\n The {} most frequent words are /n'.format(PRINT_WORDS))

Since it's a constant it's in upper case. I've also used format here since using placeholders looks a bit nicer and also is much regular than either concatenation or the % operator. But it's kind of up to preference anyway.

Lastly, the final loop uses an explicit counter, which, again, has an equivalent and nicer standard library helper called enumerate.

for i, (count, word) in enumerate(lst[:PRINT_WORDS], 1):
    print('%2s. %4s %s' % (i, count, word))

Which starts at zero normally, but I actually just saw that it supports an optional start parameter, so might as well use that.

\$\endgroup\$
  • \$\begingroup\$ Wow!!!! Thank you so much! I wish I would have used this site in class earlier in the semester. \$\endgroup\$ – Karla Richardson Apr 24 '16 at 19:44
2
\$\begingroup\$
s = open('Obama 2009.txt','r').read()  # Open the input file

What if that file doesn't exist? You would have a very nasty error. It is also recommended to use with whenever you open a file.

try:
    with open('Obama 2009.txt') as open_file:
        contents = open_file.read()
except IOError:
    print("Could not read from file")
    exit(1) # Non-zero means failure

I changed your variable name to be more clear what exactly this string is. Single-character variable names are used even by professional programmers sometimes, but not for every variable. For example, they are often used in loops, such as for i in range(len(mylist)): to get each index in a list, but any variable that is used outside of the scope in which it is defined should have a name that is indicative of what it is for.

# Program will ...

# Program assumes ...

This whole file is a program. You should treat your comments like commands to your program:

# Count the characters in text file
# Count the lines in text file
# Call split with no arguments
# Assume user has downloaded stopwords from NLTK
...

Actually, Call split with no arguments is a useless comment. The line is short; it is obvious that .split() is being called with no arguments. If you were to leave a comment, you should leave something more like:

# Split file contents by whitespace to get a list of words

Instead of reading the code to you in slightly changed language, it explains why the code was written. Anyone who knows what .split() is for should already be very clear on what it does, and a program should not be a tutorial for non-Pythoners, so I would take the comment out altogether.

d = {}
for w in words:
    if w in d:
        d[w] += 1
    else:
        d[w] = 1

Making a count of the occurrences of each item in an iterable is so common that there is a built-in module (collections) that has a class for doing that: Counter:

from collections import Counter
...
word_counts = Counter(word)

I would suggest taking a look at the documentation for each module in the standard library. Even if the module looks entirely useless now, you may need it later, and having read the documentation, you will be more likely to remember what module can do ...

num_words = sum(d[w] for w in d)

You should also take advantage of built-in methods:

num_words = sum(word_counts.values())

It still works when you use Counter because Counter is a subclass of dict.

lst = [(d[w], w) for w in d]
lst.sort()
lst.reverse()

Again, use built-ins:

lst = [(value, key) for key, value in words_count.items()]
lst.sort(reverse=True)

Yes, the first line is a little longer, but that's because I changed your naming to something more indicative of the purpose. Your lst name is also a little strange. We can see that it is a list, but what sort of list is it? In this case, it is the items in our count. Use a name that says why it is a list, not just that it is a list.

from nltk.corpus import stopwords
from nltk.tokenize import wordpunct_tokenize

Imports should go at the top of the file as mentioned by PEP 8, the Python style guide:

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

That may seem like a strange standard, but there is a very good reason: it makes it more obvious which modules are required for this program to run. That is helpful even while you are writing the program. For example, you want to use the Counter class, so you do from collections import Counter and use it. Later, you think "I want to use the Counter class here", so you do from collections import Counter and use it. This could be months after you originally wrote the program, and you forgot that you already imported it. If it were at the top, however, you would find it. When you have many imports, it is a good idea to alphebetize them to make it a little easier to see what is being imported.

stop_words = set(stopwwords.words('english')) # creating a set makes the searching faster

There you go! That's a good comment. I was wondering why you converted it to a set, and you told me.

print('\n the 30 most frequent words are /n')

Hm. You said 30, but it looks like 50 lines printed on the screen. Do I need my glasses changed or do you need your output changed? I would suggest a constant that says how many words you will print. You can then use it in both cases instead of magic numbers. Also, why do I see /n at the end of the line? Typo, I guess; it should be \n if you want a new line.

i = 1
for count, word in lst[:50]:
    print('%2s. %4s %s' %(i, count, word))
    i+= 1

% formatting strings are not officially deprecated, but it is recommended to use .format(). Your spacing is not ideal. You should be consistent. In dictionaries, you should have no space before a colon and a space after the colon. Other than that, you should either have a space on both sides or neither sides. Spacing on both sides of += is even mentioned by PEP 8:

Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).

Finally, there is already a function made for iterating through both the indices and the values in an iterable, enumerate:

for i, (count, word) in enumerate(lst[:NUM_WORDS], 1):
    print('{:>2}. {:>4} {}'.format(i, count, word)

The second argument to enumerate() is called start. Normally, enumerate() starts at zero, but we told it to start at one. The formatting strings are a little different from % format strings. Usually, you have something like {0} or {1}, etc. to refer to the different arguments passed to .format(). In this case, we are using the arguments in order, so we leave that out. What comes after the colon is the special formatting. In this case, > means right-justified and 2 or 4 mean the same as in %2s or %4s.

\$\endgroup\$
  • \$\begingroup\$ Thank you this was very helpful. When using idle are the built in functions available? I followed all of your suggestions and when the list of the top 30 words returns, the stop words are in the results/output list. \$\endgroup\$ – Karla Richardson Apr 24 '16 at 20:02
  • \$\begingroup\$ Why cant I remove the stopwords? Am I using the set created wrong? \$\endgroup\$ – Karla Richardson Apr 24 '16 at 20:08
  • \$\begingroup\$ @KarlaRichardson: Yes, they are. You can test by typing enumerate into the IDLE. You will see it print <class 'enumerate'>. If built-in functions weren't available, it would have a NameError instead. \$\endgroup\$ – zondo Apr 24 '16 at 20:09
  • \$\begingroup\$ You are creating the set correctly, but you are using it only for printing. If you actually want to change lst to everything that is not a stop word instead of printing everything that is not a stop word, change print([word for word in lst if word not in stop_words]) to lst = [word for word in lst if word not in stop_words] \$\endgroup\$ – zondo Apr 24 '16 at 20:14

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.