2
\$\begingroup\$

This my first real program in . I welcome harsh criticism. I tried to make it as efficient as possible. Trying to balance speed and readability as well. My program takes a file of text and parses and counts the frequency of the length of each word. An important specification I left out is I'm using 2.7 or earlier and not 3.x.

This example below isn't perfect for my code b/c I scale my y axis by 20's so if the frequency isn't higher then it won't be printed out. I wanted to demonstrate quickly what it was doing. Otherwise copy and paste this into a file and load it as an argument. http://www.archives.gov/exhibits/charters/declaration_transcript.html

For example:

If the this was the text that I used to parse into my file as an argument.

This would be the dictionary:

1:1, 2:5, 3:3, 4:6, 5:1, 8:1

I then take this data make a histogram. I count "&" as a word of zero length. I used a time function to determine the speed, just to give me idea of what was faster.

  #!/usr/bin/python
"""
The program reads all the input, splitting it up into words, and computes the length of each word. Punctuation marks should not be included as a part of the word, so "it's" should be counted as a three-character word, and "final." should be counted as a five-character word.
The example text includes a "word" of zero length (the "tr(x) + " | ")
"); your solution should handle this.
When all input has been processed ,the program should print a table(Histogram) showing the word count for each of the word lengths that has been encountered.  
"""


import sys
import time
from string import punctuation as punc
from string import maketrans as trans
import operator
lengthDict = {}

def insert_Dict(word):
    lengthDict[word] = lengthDict.get(word, 0) + 1

def timing(f):
    def wrap(*args):
        time1 = time.time()
        ret = f(*args)
        time2 = time.time()
        print '%s function took %0.3f ms' % (f.func_name, (time2-time1)*1000.0)
        return ret
    return wrap

def roundup(x):
    return x if x % 100 == 0 else x + 100 - x % 100


@timing
def main():  
    fileName = sys.argv[1]

    openFile = open(str(sys.argv[1]))

    readString = openFile.read()

    out = readString.translate(trans("",""), punc)

    for word in readString.split():
       if word in punc:
           #print(word)
           insert_Dict(0)

    for word in out.split():
        #print(word)
        insert_Dict(len(word))

    maxDomain = max(lengthDict.keys())
    maxRange = max(lengthDict.values())
    roundedMaxRange = roundup(maxRange)

    for x in range(roundedMaxRange,0, -20):
        newStr = ""
        if x % 100 == 0:
            newStr = newStr + str(x) + " -|"
        elif x % 20 == 0:
            newStr = newStr + "     |"
        else:
            newStr = newStr + "      " 


        for domain in range(maxDomain + 1): 
            if lengthDict.get(domain) >= x and lengthDict.has_key(domain) :
                    newStr = newStr + ('***') 
            else:
                newStr = newStr + '   '

        print(newStr)

    print('     -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-')
    print '      0',
    for x in xrange(1, 16):
        print "",
        print x, 

    print

if __name__ == '__main__':
    print('This program is being run by itself')
    main()
else:
    print('I am being imported from another module')
\$\endgroup\$
2
\$\begingroup\$

Reinderien has covered the PEP-8 style issues, so here's some input on the logic.


Running the program (with the half of your question that isn't the code as an input) gives the following result:

This program is being run by itself
100 -|                                    
     |                                    
     |                                    
     |                                    
     |      ***   ***                     
     -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-
      0  1  2  3  4  5  6  7  8  9  10  11  12  13  14  15
main function took 0.185 ms

So two obvious issues are that the X-axis should be one character further right and the numbers lose alignment.

Per the specification you should be printing a table, not a histogram.

Also, when adjacent columns are both filled it could get a bit confusing, perhaps a single-width column would be better?

100 -|                                    
     |                                    
     |                                    
     |                                    
     |       *     *                    
      -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-
       0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15

Short texts don't appear on the chart at all, and long texts could have more than 100 words of a given length - consider normalising your counts to the proportion of the input.


You are replicating functionality that already exists in Python: collections.Counter would save you a lot of work. Keys that don't have counts will automatically return zero, for example, and you don't need insert_Dict:

out = read_string.translate(None, punc)
length_dict = Counter(map(len, out.split(" ")))  

Note that explicitly splitting on a single space means you don't have to handle the single punctuation characters separately:

>>> "foo & ba'r".translate(None, punc).split(" ")
['foo', '', 'bar']

Your main should look more like:

def main():
    with open(sys.argv[1]) as f:
        data = f.read()
    counts = create_counter(data)
    proportions = normalise_counts(counts) # if required
    draw_histogram(proportions)

Splitting the other functionality out into separate functions.


Your histogram-drawing code uses a lot of explicit string concatenation. It is better to .format and .join strings (see string methods), for example:

print '       {0}'.format("".join(map("{0:<3}".format, range(16))))

Which produces:

       0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15 
\$\endgroup\$
  • \$\begingroup\$ Sorry about the confusion on histogram. It suppose to be a histogram and my example is poor because I scale my y axis by 20 so it doesn't show up. I use declaration as text I parse if that helps. \$\endgroup\$ – user3590149 Sep 11 '14 at 13:09
  • \$\begingroup\$ You given me a lot of good stuff. I will take some time to digest it all. \$\endgroup\$ – user3590149 Sep 11 '14 at 13:10
0
\$\begingroup\$
  • Use with on your open to clean up even if there's an application exception
  • If you're in Python 2.6 or later, just pass None for the first argument to translate
  • insert_Dict should be lowercase, and since it doesn't accept a word (but rather a word_length), fix its argument naming
  • Add a space after roundedMaxRange,
  • newStr = newStr + should be replaced by new_str +=
  • drop the trailing commas on your print statements
  • drop the parentheses around your print statements (or make them all have it; either way, make it uniform)
  • replace print '' with print
\$\endgroup\$
  • 1
    \$\begingroup\$ Trailing commas have a meaning in the print statement: omit the newline character. \$\endgroup\$ – Janne Karila Sep 11 '14 at 5:25
  • \$\begingroup\$ That's odd - I tried it in my interpreter and they didn't do that. \$\endgroup\$ – Reinderien Sep 11 '14 at 12:33
  • \$\begingroup\$ I'm not sure if you can do that with 3.x python. I'm using 2.x and the trailing comment works. \$\endgroup\$ – user3590149 Sep 11 '14 at 13:13

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.