1
\$\begingroup\$

I created a program in python that reads a file and outputs a frequency graph of the words it contains. Any feedback or tips would be appreciated.

from re import split
from collections import Counter

size = int(input("Output window size(in chars): "))
with open("text.txt", "r") as f:
  words = [w for w in split(r"[\W_\d]+", f.read().strip()) if w]
if not words:
  print("Put some text in `text.txt`!")
  quit()
word_count = Counter(words)
top_words = sorted(word_count.keys(), key = lambda w : word_count[w], reverse = True)
scale = (size - len(max(top_words, key = len))) / word_count[top_words[0]]
for word in top_words[:10]:
  print("-" * (int(word_count[word] * scale) - 2) + "| " + word)
\$\endgroup\$
3
\$\begingroup\$

Review

  • The Python style guide [PEP8] advises to use 4 space indentation
  • You don't have to sort the words

    Counter has some features that allow you to get the most common occurrences, aptly named most_common()

  • String formatting is cleaner vs manual string appending

    Instead of manually concatenating use str.format() or even f"{string}" with Python3.6+

Code

Using these additions we can rewrite your last code block into:

word_count = Counter(words)
scale = (size - max(map(len, word_count))) / word_count.most_common(1)[0][1]
for word, occurence in word_count.most_common(10):
    print("{}| {}".format("-" * int(occurence * scale - 2), word))
\$\endgroup\$
  • 1
    \$\begingroup\$ Wouldn't max(len(word) for word in word_count) or max(map(len, word_count)) be a lot easier than calling max with len as the key and then calling len on it again? \$\endgroup\$ – Graipher Mar 13 at 10:08
  • 1
    \$\begingroup\$ Ohh I like max(map(len, word_count)) didn't cross my mind to write it this way! \$\endgroup\$ – Ludisposed Mar 13 at 10:09
  • 1
    \$\begingroup\$ You want to scale to the longest word in top ten, not the longest word in the whole corpus. Also .most_common is presumably \$O(n)\$ in the number of unique words; it would be better to retrieve the top ten once and take the top one from there. \$\endgroup\$ – Oh My Goodness Mar 13 at 14:09
  • \$\begingroup\$ @OhMyGoodness "You want to scale to the longest word in top ten, not the longest word in the whole corpus" I agree, but this is not done by OP. It leaves me guessing at their intentions, and I didn't want to deviate from OP's code behavior. \$\endgroup\$ – Ludisposed Mar 13 at 15:40
  • \$\begingroup\$ @Ludisposed OP's code is len(max(top_words, key = len))) and it selects longest length from within top_words. \$\endgroup\$ – Oh My Goodness Mar 13 at 18:07

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.