patternpythonMinor
Uses game API to post stats about user when requested
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
```
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
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.
I.e. everything between lines 15 and 25 should be in a function (or
multiple), e.g.
Style
doesn't say much, so maybe
instead of
-
You already mentioned the slightly inconsistent use of quotation
marks; I think the same goes for imports and whitespace use.
-
If you can, be Python 3 compatible. You also might want to run
or other tools (I won't list the output here, but see
PEP8 for more).
This means that that code block is only executed when the file is
invoked as a script instead of being
just good practice. See
the documentation
for a short blurb.
-
The
exception handler, i.e.
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
exception handlers. Together that would look like this:
-
Less repetition. It's not forbidden to assign common subexpressions
to a temporary name. E.g.
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.
-
You already have constants at the top, so move other constants there
as well. E.g.
least. (Again, PEP8.)
Problems, Ideas
That makes no sense to me unless they have a zero prefixed or so.
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).
(
output? Quite optional, but if the program gets larget I'd definitely
look into that.
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; dicinstead 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
pep8or 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 isjust good practice. See
the documentation
for a short blurb.
-
The
isinstance in that block should also rather be a secondexception handler, i.e.
except requests.HTTPError as e. OtherwiseI'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 otherexception 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 usetimes afterwards. That will also go a long way to reduce thehorizontal 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 atleast. (Again, PEP8.)
Problems, Ideas
- Strings are parsed to numbers with
intjust to beformatted 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
stroperations, 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 standardoutput? Quite optional, but if the program gets larget I'd definitely
look into that.
Code Snippets
...
import sys
import os
...
from datetime import datetime, timedeltadef 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()
continueContext
StackExchange Code Review Q#88060, answer score: 7
Revisions (0)
No revisions yet.