HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Uses game API to post stats about user when requested

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
userstatsusespostgameaboutwhenapirequested

Problem

First Python script, so my code is pretty laughable. Sensitive information has been redacted. One thing that I'd like to point out is that there is an inconsistent use of '' and "". This is an old habit that I really need to break.

```
import sys, os
import praw
import time
import sqlite3
import re
import requests
import json
from datetime import datetime,timedelta

USERNAME = ""
PASSWORD = ""

time_zone = updateTime = datetime.utcnow() - timedelta(hours=7)
time_stamp = updateTime.strftime("%m-%d-%y %I:%M:%S %p PST :: ")

sql = sqlite3.connect((os.path.join(sys.path[0],'redacted-sql.db')))

cur.execute('CREATE TABLE IF NOT EXISTS oldmentions(ID TEXT)')
sql.commit()

r = praw.Reddit()
r.login(USERNAME, PASSWORD)

def stats():
mentions = list(r.get_mentions(limit=None))
unreads = list(r.get_unread(limit=None))
for mention in mentions:
mid = mention.id

try:
pauthor = mention.author.name
except AttributeError:
#author is deleted
continue

cur.execute('SELECT * FROM oldmentions WHERE ID=?', [mid])
if cur.fetchone():
#post already in database
continue

pbody = mention.body.lower().replace('\n', ' ').encode('utf-8')
pbody_strip_1 = re.sub(r'[^A-Za-z0-9 ]+', '', str(pbody_strip_0))
pbody_words = pbody_strip_1.split(' ')
pbody_words_1 = filter(None, pbody_words)
try:
if pbody_words_1[pbody_words_1.index('redactedbot')-1] == "u":
charname = pbody_words_1[pbody_words_1.index('redactedbot')+1]
else:
cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
sql.commit()
mention.mark_as_read()
continue
except (IndexError, KeyError, ValueError):
cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
sql.commit()
mention.mark_as_read()
continue

cns_char_dic = j

Solution

At first glance the code looks good, though I'd suggest a number of
changes to make it a bit nicer.

Structure

  • You have one main function which is a humongous ~160 lines long. I'd


rather see lots of smaller, more manageable functions. That could
e.g. be functions to format text, extract values, calculate numbers,
etc. Just have some kind of split between blocks of functionality so
you can actually see discrete functionality on its own. For example,
I'd definitely move the inner part of the loop into a separate
function, as well as the final message construction.

  • Even though this is a script, the same goes for configuration.


I.e. everything between lines 15 and 25 should be in a function (or
multiple), e.g. configure or so.

Style

  • The variable names could be better / more descriptive. stats


doesn't say much, so maybe statistics_loop, or whatever; dic
instead of dict is really confusing.

-
You already mentioned the slightly inconsistent use of quotation
marks; I think the same goes for imports and whitespace use.

...
import sys
import os
...
from datetime import datetime, timedelta


-
If you can, be Python 3 compatible. You also might want to run pep8
or other tools (I won't list the output here, but see
PEP8 for more).

  • The part at the bottom of the script should in general be wrapped in


if __name__ == "__main__": in order to be loadable as a module.
This means that that code block is only executed when the file is
invoked as a script instead of being imported as a module. This is
just good practice. See
the documentation
for a short blurb.

-
The isinstance in that block should also rather be a second
exception handler, i.e. except requests.HTTPError as e. Otherwise
I'd go so far as to say not to catch all exceptions, since the default
will already print the stacktrace as you do here manually. Also, the
requests isn't needed and it's already skipped in the other
exception handlers. Together that would look like this:

def main():
    try:
        stats()
    except HTTPError:
        print(time_stamp + 'A site/service is down. Probably Reddit.')
...
if __name__ == "__main__":
    main()


-
Less repetition. It's not forbidden to assign common subexpressions
to a temporary name. E.g.
times = census_char_dic['character_list'][0]['times'] and then use
times afterwards. That will also go a long way to reduce the
horizontal extent of the code to below the (soft) 80 character limit.

-
Less repetition. Repeated code fragments go into functions, or should
be rewritten so they don't repeat. E.g. lines 59 and 65. I also
don't particularly like exceptions for these things, but it might
possibly be the easier option.

def find_character_name(pbody_words_1):
    try:
        bot_index = pbody_words_1.index('redactedbot')
        if pbody_words_1[bot_index-1] == "u":
            return pbody_words_1[bot_index+1]
    except (IndexError, KeyError, ValueError):
        pass
...
charname = find_character_name(pbody_words_1)
if charname is None:
    print(time_stamp + mid + " is not valid. Adding to database anyway.")
    cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
    sql.commit()
    mention.mark_as_read()
    continue


-
You already have constants at the top, so move other constants there
as well. E.g. servers_dic (which should be named SERVERS_DIC at
least. (Again, PEP8.)

Problems, Ideas

  • Strings are parsed to numbers with int just to be formatted again.


That makes no sense to me unless they have a zero prefixed or so.

  • Also, int("0"), wtf? That will always be zero.



  • The code is using lots of str operations, are all of those


necessary? I'd also suggest, as you're formatting a lot of text, to
use templating library instead, preferrably with some shortcuts for
e.g. the plural suffixes (or you know, create a function for that).

  • Maybe add logging


(import logging) for debugging purposes instead of using standard
output? Quite optional, but if the program gets larget I'd definitely
look into that.

Code Snippets

...
import sys
import os
...
from datetime import datetime, timedelta
def main():
    try:
        stats()
    except HTTPError:
        print(time_stamp + 'A site/service is down. Probably Reddit.')
...
if __name__ == "__main__":
    main()
def find_character_name(pbody_words_1):
    try:
        bot_index = pbody_words_1.index('redactedbot')
        if pbody_words_1[bot_index-1] == "u":
            return pbody_words_1[bot_index+1]
    except (IndexError, KeyError, ValueError):
        pass
...
charname = find_character_name(pbody_words_1)
if charname is None:
    print(time_stamp + mid + " is not valid. Adding to database anyway.")
    cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
    sql.commit()
    mention.mark_as_read()
    continue

Context

StackExchange Code Review Q#88060, answer score: 7

Revisions (0)

No revisions yet.