5
\$\begingroup\$

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()
\$\endgroup\$
  • \$\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 May 16 '15 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 May 28 '15 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 May 29 '15 at 10:31
7
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ @vtesting Sure, I've added examples. For the concatenation of course use whatever is necessary, I was concerned about the huge number of strs; however using .format might be more consistent / easier than dealing with those conversions manually. \$\endgroup\$ – ferada Apr 27 '15 at 13:15
  • \$\begingroup\$ Okay, @vtesting I've started here, but there's still stuff to do. If you want, clone it, because I'll delete it in a few days. \$\endgroup\$ – ferada Apr 28 '15 at 18:33
  • \$\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 Apr 29 '15 at 10:32

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.