4
\$\begingroup\$

I have a list of keywords and I want to count the number of records in a db which contain any one of these keywords. From each record, I want to consider only title and description to be checked.

Is this correctly written?

self.keyword_list = ['Buzz', 'Heard on the street', 'familiar with the development', 'familiar with', 'enters race', 'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged', 'fabricated', 'inspection', 'inspected', 'to monetise', 'cancellation', 'control', 'pact', 'warning', 'IT scanner', 'Speculative', 'Divest', 'Buzz', 'Heard on the street', 'familiar with the development', 'familiar with the matter', 'Sources', 'source', 'Anonymous', 'anonymity', 'Rumour', 'Scam', 'Fraud', 'In talks', 'Likely to', 'Cancel', 'May', 'Plans to ', 'Raids', 'raid', 'search', 'Delisting', 'delist', 'Block', 'Exit', 'Cheating', 'Scouts','scouting', 'Default', 'defaulted', 'defaulter', 'Calls off', 'Lease out', 'Pick up', 'delay', 'arrest', 'arrested', 'inks', 'in race', 'enters race', 'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged', 'fabricated', 'inspection', 'inspected', 'monetise', 'cancellation', 'control', 'pact', 'warning', 'IT scanner', 'Speculative', 'Divest']
fetch_record = self.collection1.find()
for record in fetch_record:
    count2 += 1
    for item in self.keyword_list:
        if item.lower() in u'{} {}'.format(record['title'], record['description']).lower():
            #print "Matched for item : ", item
            count1 += 1
            break 
print "Matched records are : ", count1
print "Total records are : ", count2
\$\endgroup\$
4
\$\begingroup\$

A few things that meet my eye:

Value Casing

You run item.lower() for each item in the keyword list when you iterate over the records. You could ensure the lower-case of all items in your keyword-list before getting to the records, then that step would be away.

Naming

count2 should be total, count1 should be matched, collection1 needs a better name. Names are vital in allowing other programmers to understand your code.

Nitpicks

  • You may get better performance by formatting the string before entering the loop over the keyword-list
  • The keyword list contains duplicate items
  • Remove Code you do not need instead of commenting it out. Version Control has got your back :)
  • keyword_list could be keywords...
  • printing is not really a valid return from a method.
\$\endgroup\$
4
\$\begingroup\$

The Python style (PEP0008) guide recommends that you keep to a maximum of 79 characters per line, for readability's sake. You keyword list blows right past this. You should split it over multiple lines. Whitespace matters in Python but splitting the line is easy when you have a list items between brackets ([], {} or ()).

self.keyword_list = ['Buzz', 'Heard on the street',
    'familiar with the development', 'familiar with', 'enters race',
    'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes',
    'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged',
    'fabricated', 'inspection', 'inspected', 'to monetise',
    'cancellation', 'control', 'pact', 'warning', 'IT scanner',
    'Speculative', 'Divest', 'Buzz', 'Heard on the street',
    'familiar with the development', 'familiar with the matter',
    'Sources', 'source', 'Anonymous', 'anonymity', 'Rumour', 'Scam',
    'Fraud', 'In talks', 'Likely to', 'Cancel', 'May', 'Plans to ',
    'Raids', 'raid', 'search', 'Delisting', 'delist', 'Block', 'Exit',
    'Cheating', 'Scouts','scouting', 'Default', 'defaulted', 'defaulter',
    'Calls off', 'Lease out', 'Pick up', 'delay', 'arrest', 'arrested',
    'inks', 'in race', 'enters race', 'mull', 'consider', 'final stage',
    'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege',
    'alleges', 'alleged', 'fabricated', 'inspection', 'inspected',
    'monetise', 'cancellation', 'control', 'pact', 'warning',
    'IT scanner', 'Speculative', 'Divest']

Also now that I've done this I notice that there are duplicates in here. 'familiar with the development' is in there twice, but even so shouldn't 'familiar with' match that regardless? Likewise 'eye' would match 'eyes'. You could save some iteration if you checked over for these duplicates as they're just adding redundant loops. Also since you're running break as soon as you find one, it would be a performance boost if you could arrange the order so that more common words are earlier (assuming you know what keywords are more common).

Also as a nitpick, you shouldn't print string, value, it's not a good format. Instead use str.format.

print ("Matched records are : {}".format(matched))
print ("Total records are : {}".format(total))

The format function replaces instances of {} with paramaters that it's passed and is very useful as it has a lot of available syntax. If you get used to it, it's easier to take advantage of later on.

Shouldn't count2 just be set as len(fetch_record)? You seem to increment it every single time anyway. (I also agree about choosing better names but I wont retread that ground).

fetch_record = self.collection1.find()
count2 = len(fetch_record)

Note: The following suggestions are a bit less readable because of how much is being put into one line of code. However they are useful performance optimisations and they will show you about some interesting Python techniques that could be useful elsewhere anyway.

Using the any operator you can eliminate your second for loop. any takes a generator expression, which is basically like a for loop collapsed into a one line expression. any will return True if it finds any condition in the loop to be true. So you could rewrite your inner for loop as this:

if any(item.lower() in u'{} {}'.format(record['title'],
                                      record['description']).lower()
            for item in self.keyword_list):
    count1 += 1

Also, Python can actually evaluate booleans as integers. So if you were to do count1 += True that would actually add 1 to count1, while False would add 0. Using that you don't need an if statement, you can just directly add the boolean result to count1.

for record in fetch_record:
    count1 += any(item.lower() in u'{} {}'.format(record['title'],
                                      record['description']).lower()
                  for item in self.keyword_list)

Now we can actually condense this into a single line, getting rid of the other for loop too, using the sum function. It's another function that takes a generator expression, and returns an integer of the sum of all values found in it. This can work on booleans so you could just get the sum of your any expression for each element in fetch_record. Readability gets tough here, but adding a comment or two would clear it up a bit.

#Sum of the records which have any keyword in their title/description
count1 = sum(any(item.lower() in u'{} {}'.format(record['title'],
                 record['description']).lower()
                 for item in self.keyword_list)
             for record in fetch_record)

My comment reuses the function names because I think that makes it easier for you to read the comment and see how the code is actually performing it.


So this is how I'd rewrite your full script:

self.keyword_list = ['Buzz', 'Heard on the street',
    'familiar with the development', 'familiar with', 'enters race',
    'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes',
    'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged',
    'fabricated', 'inspection', 'inspected', 'to monetise',
    'cancellation', 'control', 'pact', 'warning', 'IT scanner',
    'Speculative', 'Divest', 'Buzz', 'Heard on the street',
    'familiar with the development', 'familiar with the matter',
    'Sources', 'source', 'Anonymous', 'anonymity', 'Rumour', 'Scam',
    'Fraud', 'In talks', 'Likely to', 'Cancel', 'May', 'Plans to ',
    'Raids', 'raid', 'search', 'Delisting', 'delist', 'Block', 'Exit',
    'Cheating', 'Scouts','scouting', 'Default', 'defaulted', 'defaulter',
    'Calls off', 'Lease out', 'Pick up', 'delay', 'arrest', 'arrested',
    'inks', 'in race', 'enters race', 'mull', 'consider', 'final stage',
    'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege',
    'alleges', 'alleged', 'fabricated', 'inspection', 'inspected',
    'monetise', 'cancellation', 'control', 'pact', 'warning',
    'IT scanner', 'Speculative', 'Divest']

records = self.collection1.find()
total = len(records)
matched = sum(any(item.lower() in u'{} {}'.format(record['title'],
                  record['description']).lower()
                  for item in self.keyword_list)
              for record in records)

print ("Matched records are : {}".format(matched))
print ("Total records are : {}".format(total))
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd also sort the keyword list for readability - it makes it obvious if you have duplicates then \$\endgroup\$ – Tom Tanner Sep 11 '15 at 11:13
1
\$\begingroup\$

There are multiple performance issues that are easy to correct:

  • keyword list should be a set, to make it impossible to add duplicated
  • judging by the self. prefixes, it seems we're inside a class method. In that case, it would be better to move the keyword set definition outside, rather than recreating on every method call
  • for each record, you lowercase the full set of keywords. It would be better to lowercase the keywords once, before iterating over the records
  • for each keyword, you lowercase the target fields in the record. It would be better to lowercase first before checking the keywords

Numbering in variable names is never OK when there are better alternatives. I recommend matched_count and total_count.

Finally, the fetch_record variable seems quite pointless. I would inline it.

\$\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.