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 = json.loads(cns_char_j)
char_exist = cns_char_dic['returned']
if char_exist != 1:
cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
sql.commit()
mention.mark_as_read()
continue
char_case = cns_char_dic['person_list'][0]['name']['first']
char_id = cns_char_dic['person_list'][0]['person_id']
char_creation = time.strftime(time_format, time.localtime(float(cns_char_dic['person_list'][0]['times']['creation'])))
char_login = time.strftime(time_format, time.localtime(float(cns_char_dic['person_list'][0]['times']['last_login'])))
char_login_count = int(float(cns_char_dic['person_list'][0]['times']['login_count']))
char_h, char_m = divmod(int(cns_char_dic['person_list'][0]['times']['minutes_played']), 60)
if char_h == 1:
hours = " hour "
else:
hours = " hours "
if char_login_count == 1:
logins = " login)"
else:
logins = " logins)"
char_rank = cns_char_dic['person_list'][0]['battle_rank']['value']
post_reply_rank = "Battle rank: " + char_rank
if char_rank_next != "0":
post_reply_rank += " (" + char_rank_next + "% to next)"
char_faction = cns_char_dic['person_list'][0]['faction']
char_world_id = cns_char_dic['person_list'][0]['world_id']
try:
char_outfit = cns_char_dic['person_list'][0]['outfit_member']
if char_outfit['member_count'] != "1":
post_reply_outfit = "Outfit: [" + str(char_outfit['alias']) + "] " + str(char_outfit['name']) + " (" + "{:,}".format(int(char_outfit['member_count'])) + " members)"
else:
post_reply_outfit = "Outfit: [" + char_outfit['alias'] + "] " + char_outfit['name'] + " (" + char_outfit['member_count'] + " member)"
except KeyError:
post_reply_outfit = "Outfit: None"
try:
char_kills = cns_char_dic['person_list'][0]['stats']['stat_history'][5]['all_time']
char_deaths = cns_char_dic['person_list'][0]['stats']['stat_history'][2]['all_time']
cns_stat_j = cns_stat.text
cns_stat_dic = json.loads(cns_stat_j)
char_stat = cns_stat_dic['persons_stat_list']
if pauthor.lower() != USERNAME.lower():
try:
mention.reply(post_reply)
mention.mark_as_read()
except APIException:
pass
cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
sql.commit()
else:
print(time_stamp + 'Will not reply to myself')
cur.execute('INSERT INTO oldmentions VALUES(?)', [mid])
sql.commit()
mention.mark_as_read()
-
\$\begingroup\$ I've rollback your edit. Deleting the content of the question is not acceptable. If you have a problem that require deletion of this question, talk with the moderators (flag your question). Unless you have a good reason, you should delete your question. \$\endgroup\$Marc-Andre– Marc-Andre2015年05月16日 14:51:27 +00:00Commented May 16, 2015 at 14:51
-
\$\begingroup\$ Unless you have a really good reasons to delete this post (read this is proprietary code from a company and you did not have the permission to post it), the question will most likely not be deleted. You can still have the post dissociate from your account. You could bring this to meta.codereview.stackexchange.com but if they rejected your flag I don't think there is much you can do. \$\endgroup\$Marc-Andre– Marc-Andre2015年05月28日 13:37:32 +00:00Commented May 28, 2015 at 13:37
-
\$\begingroup\$ Deleting answered questions is rarely done. See How does deleting work. In this case, deleting your question would remove 60 rep or so from @ferada too, and that would be unfair. \$\endgroup\$rolfl– rolfl2015年05月29日 10:31:36 +00:00Commented May 29, 2015 at 10:31
1 Answer 1
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 maybestatistics_loop
, or whatever;dic
instead ofdict
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 beingimport
ed 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, therequests
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 usetimes
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 namedSERVERS_DIC
at least. (Again, PEP8.)
Problems, Ideas
- Strings are parsed to numbers with
int
just to beformat
ted 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.
-
\$\begingroup\$ @vtesting Sure, I've added examples. For the concatenation of course use whatever is necessary, I was concerned about the huge number of
str
s; however using.format
might be more consistent / easier than dealing with those conversions manually. \$\endgroup\$ferada– ferada2015年04月27日 13:15:48 +00:00Commented Apr 27, 2015 at 13:15 -
-
\$\begingroup\$ Sorry, I don't have the time to do all the changes I proposed. Yes, I'd recommend choosing either (for consistency) text concatenation or
.format
, or, for the longer text, a templating library. \$\endgroup\$ferada– ferada2015年04月29日 10:32:24 +00:00Commented Apr 29, 2015 at 10:32