3
\$\begingroup\$

How do I improve the below code, which takes input from a file and does some word analysis? I find a lot of redundancy in it. I want to know a better way of implementing it.

analysis = {}
wordanalysis = {}
found = True 

for line in sys.stdin:
   (n1,n2,n3,n4,n5,n6) = re.split("\t+",line.strip())
   tweet = re.split("\s+", n6.strip().lower())
   #adding tweet id to a dictionary

   for i in range(0, len(tweet)):
     print "{", n1, wordanalysis,"}"
     wordanalysis["word"] = tweet[i]
     wordanalysis["charcount"]= len(tweet[i])

     if len(tweet[i]) > 7:
        wordanalysis["longword"] = found
     else:
        wordanalysis["longword"] = not(found)
     #if 1st char is UpperCase
     if tweet[i].istitle():
        wordanalysis["title"] = found
     else:
        wordanalysis["title"] = not(found)

     if tweet[i].isupper():
        wordanalysis["uppercase"] = found
     else:
        wordanalysis["uppercase"] = not(found)
\$\endgroup\$
4
\$\begingroup\$
  1. I would wrap the entire part of analyzing in a function:

    def analyze_twees(tweets):
    

    Then you could pass in content, from whichever source. It doesn't matter, whether the input comes from sys.stdin or a string, which you just generated. This makes the function easier to test, since you do not need passing a file into your function.

  2. As 200_success said,

    for i in range(0, len(tweet)):
    

    is considered poor style in Python. According to (1) you should iterate over the tweets as suggested:

    for tweet in tweets.
    
  3. Your variable naming is very obscure. Think of future you coming up in 6 Months fixing a bug - do you understand your code at first glance? I doubt that.

    n6.strip().lower()
    

    One can only guess, what this is about. But if you wrote something like

    tweeted_text = re.split("\s+", tweet.strip().lower())
    

    one could imagine, what you intended to do.

  4. You should split your concerns:

    1. you are going to read a file line by line. So that should be one function.
    2. you are printing something to the console. That would be another function.
    3. you are analyzing the tweeted text. That would be the third function.

      def print_tweet_analysis(dataset):
          for row in dataset:
              console_out(analyze(row))
      

      That would be your complete algorithm in three lines of code. If you come in 10 years from now back to your code, you unterstand, what this script is about: it takes data, analyzes it and puts the result to the console. If you want to know, what analyze means, you look at the function and will see. The same goes for your analyze function. You could fine grain your logic into small pieces, each sitting on the top of the other and if you read the code, you drill down as far as necessary to understand the code and maybe finding later on bugs and fixing those.

  5. Instead of using a dictionary for your algorithm, I would recommend namedtuple. You could easily define your analysis-tuple as

    Analysis = namedtuple("Analysis", "word charcount longword title uppercase")
    

    and return simply:

    return Analysis(tweet, len(tweet), len(tweet)>7, tweet.istitle(), tweet.isupper())
    

    And you are done.

    If you wrote your code like this, everybody including future you would know, what this is all about.

    One thing, I still do not understand is:

    You are defining tweet as

    tweet = re.split("\s+", n6.strip().lower())
    

    iterating over it and checking for

    tweet[i].isupper()
    

    Am I overseeing something or does this make no sense only to me?

\$\endgroup\$
  • \$\begingroup\$ Very helpful. Thanks a lot. How do I pass the lines that I am reading from standard input between functions? That is the only reason I am not using functions. \$\endgroup\$ – fscore Nov 3 '13 at 21:26
  • \$\begingroup\$ Like any other variable. You could iterate over it like over any other iterable. It is even possible to use something in other languages called stringstream c.f. docs.python.org/2/library/stringio.html \$\endgroup\$ – Thomas Junk Nov 3 '13 at 22:48
2
\$\begingroup\$

First, I see some odd behaviour that seems buggy, and if it is intended then it deserves a comment:

  • Variables n2, n3, n4, and n5 are never used. A comment with some sample lines of the expected input would be very helpful.
  • When you print wordanalysis, you are printing the analysis from the previous iteration of the loop.

The main simplification to apply is that the tests themselves are boolean values, and can therefore be assigned directly.

wordanalysis["longword"] = (len(tweet[i]) > 7)
# Is 1st char UpperCase?
wordanalysis["title"] = tweet[i].istitle()
wordanalysis["uppercase"] = tweet[i].isupper()

Furthermore, I would iterate over the list items rather than over the indexes of the list.

tweets = re.split("\s+", n6.strip().lower())
for tweet in tweets:
    ...
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. I really like this one: wordanalysis["longword"] = (len(tweet[i]) > 7) They are boolean values so that makes total sense. \$\endgroup\$ – fscore Nov 4 '13 at 0:22

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.