4
\$\begingroup\$

Can somebody help me make the code below smarter and free of redundancy? The code does exactly what I want, but I believe there are many redundant loops and arrays.

from collections import Counter
import csv
import re

with open("/Users/max/train.csv", 'r') as file:
  reviews = list(csv.reader(file))

def get_texts(reviews, score):
  texts = [] 
  texts.append([r[0].lower() for r in reviews if r[1] == str(score)]) ; 
  return texts

def getWordListAndCounts(text):
  words = []  
  for t in text:
      for tt in t:
        for ttt in (re.split("\s+", str(tt))):
            words.append(str(ttt))
  return Counter(words)

negative_text_list = get_texts(reviews, -1)
positive_text_list = get_texts(reviews, 1)

negative_wordlist_counts = getWordListAndCounts(negative_text_list)

positive_wordlist_counts = getWordListAndCounts(positive_text_list)

print("Negative text sample: {0}".format(negative_text_list))
print("Negative word counts: {0}".format(negative_wordlist_counts))
print("Positive text sample: {0}".format(positive_text_list))
print("Positive word counts: {0}".format(positive_wordlist_counts))

Sample contents in train.csv are as below.

i like google,1
google is evil,-1
Apple is cute,1
Microsoft booo,-1
\$\endgroup\$
4
\$\begingroup\$

Your code is actually really good. But:

  • getWordListAndCounts should be get_word_list_and_counts. This is as Python has a style guide called PEP8.

  • You use a ; but for no reason. It's best to never use it.

  • In get_texts you do [].append([...]). This can be: [[...]].

  • In get_texts you have r, it would be better to use review, this makes it easier to read. Also all the ts in getWordListAndCounts.

  • You can make getWordListAndCounts a list/generator comprehension.

  • You seem to mix spaces and tabs. This leads to highly breakable code. (Look at your question)

  • Don't have white space after code. It's really annoying, and violates PEP8.

Overall I would say your code is pretty decent.

I would re-write getWordListAndCounts to this for example:

def get_word_list_and_counts(text):
    # Rename the `t`'s I have no context on what they are.
    return Counter(
        str(ttt)
        for t in text
        for tt in t
        for ttt in (re.split("\s+", str(tt)))
    )

To remove the [].append, you can use the following if it has to be a list.

def get_texts(reviews, score):
    return [r[0].lower() for r in reviews if r[1] == str(score)]

As no-one has said it:

[[...]] in get_texts seems redundent, adds complexity, and makes the algorithm worse in memory, from \$O(1)\$ with generator comprehensions, to \$O(n)\$.

The way you call the code shows us also that there is potential to reduce the complexity.

getWordListAndCounts(get_texts(reviews, 1))

To show the change to get \$O(1)\$:

def get_texts(reviews, score):
    return (
        review[0].lower()
        for review in reviews
        if review[1] == str(score) # use ferada's change
    )

def get_word_list_and_counts(text):
    return Counter(
        word
        for line in text
        for word in (re.split("\s+", line))
    )

The above shows you can clearly see it's capable to be in a single function, if you wish for it. And remove one of the fors in the get_word_list_and_counts.

It as one function would look like this:

def get_word_list_and_counts(reviews, score):
    return Counter(
        word
        for review in reviews
        if review[1] == str(score)
        for word in re.split("\s+", review[0].lower())
    )

That with ferada's change is probably the smallest and most concise you'll get it. Also the second for statement will only run if the if is true.

\$\endgroup\$
  • \$\begingroup\$ Thanks Joe, quite helpful. Actually in get_texts, I am trying to avoid getting [[...]] . How can I change [].append([...]). ? Then I will be able to reduce the number of for loops in getWordListAndCounts. \$\endgroup\$ – Max Oct 9 '15 at 12:18
  • 1
    \$\begingroup\$ @Max [].append([...]) is the same as [[...]]. You can remove the outer [], so that it becomes [...]. Or as I put in the answer, (...), if memory is a concern. (Replace ... with r[0].lower() for r in reviews if r[1] == str(score)) \$\endgroup\$ – Peilonrayz Oct 9 '15 at 12:23
3
\$\begingroup\$

I agree, looks good. It would really help to have some sample input btw.

  • I can't tell whether either of the strs in get_word_list_and_counts are necessary. Even with something like get_word_list_and_counts([["hue hue hue"], [1, 2]]) I get a result like Counter({'hue': 3, '2': 1, '1': 1}), which is the same as if the [1, 2] was instead "1 2".

The following complaints are mostly micro-optimisations, but it wouldn't hurt to keep them in mind for future projects.

  • get_texts is repeatedly calling str, that can be avoided by storing the string representation or passing in a string in the first place.

    def get_texts(reviews, score):
        score = str(score)
        return [[r[0].lower() for r in reviews if r[1] == score]]
    
  • There's no reason not to do the whole process in a loop while reading from the CSV file. That will keep memory consumption down and make the program work even on much bigger files. E.g.:

    with open("...") as file:
        for line in csv.reader(file):
            # split line
            # update counter
    
\$\endgroup\$

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.