2
\$\begingroup\$

This is the revised code after incorporating the suggestions from user Roman Susi.
Link to the previous post here.

Notable edits: I have factored out the bigger functions into smaller tasks. The functions and variables have(hopefully) less ambiguous names, and the JSON object is now deepcop[ied].

I am still interested in making the code more Pythonic, factoring out the functions if need be, and wondering if my use of deepcopy is appropriate. I am open to any other suggestions.

Updated Code:

#!/usr/bin/python3
from copy import deepcopy
import json
import os
import praw

"""
Scrapes posts for comments and saves the comment ID and comment text to the
accompanying JSON file.
"""

JSON_DEFAULT_LOC = os.environ.get('JSON_LOCATION') or os.path.join(
    os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
    "survey_response.json")


def login():
    """ Create a reddit instance according to praw.ini configuration.
    Call reddit.user.me() as a check.
    Return reddit instance.
    """
    print("Authenticating")
    reddit = praw.Reddit("appname", user_agent="Chrome:com.example.bot-"
                                                "name:v1 (by /u/)")
    print("Authenticated as {}".format(reddit.user.me()))
    return reddit


def load_survey_responses(fp=None):
    """ Load the current survey response file as a JSON object.
    """
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "r") as file:
        survey_results = json.load(file)
    return survey_results


def scrape_submissions(reddit):
    """ For each post specified in survey_responses.json, scrape the comments
    within the post and write the new or updated responses to the JSON dict.
    """
    survey_responses = load_survey_responses()
    shortlink_id_list = extract_shortlink_id(survey_responses)

    for sl_id in shortlink_id_list:
        submission = reddit.submission(id=sl_id)
        try:
            updated_responses = prepare_survey_update(submission,
                                                      sl_id,
                                                      survey_responses)
            write_comments_to_file(updated_responses)
            print("Comments from '{}' "
                  "saved to file".format(submission.subreddit))
        except OSError as e:
            print("Could not write to file: {}".format(e))


def extract_shortlink_id(survey_responses):
    return [survey_responses[sub][0]["shortlink"][-6:]
            for sub in survey_responses]


def prepare_survey_update(submission, sl_id, survey_responses):
    """ Combs through a submission's comments and replaces the "more comments",
    extracts the comments from the submission, and prepares file to be written
    with an updated JSON dictionary.
    """
    submission_replaced = replace_more_comments(submission)
    submission_comment = extract_submission_comment(submission_replaced)
    updated = update_response(survey_responses, submission_comment, sl_id)
    return updated


def replace_more_comments(submission):
    submission.comments.replace_more(limit=0)
    return submission


def extract_submission_comment(submission_replaced):
    submission_comment = [{comment.id: comment.body}
                          for comment in submission_replaced.comments.list()]
    return submission_comment


def update_response(old_responses, comment_to_add, sl_id):
    new_responses = deepcopy(old_responses)
    for i in new_responses:
        if sl_id in new_responses[i][0]["shortlink"]:
            new_responses[i][1]["responses"] = comment_to_add
    return new_responses


def write_comments_to_file(updated_json, fp=None):
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "w") as fp:
        json.dump(updated_json, fp)


def main():
    reddit = login()
    scrape_submissions(reddit)


if __name__ == "__main__":
    main()
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

This is much clearer now (hopefully, you also agree).

Some more ideas:

It is sometimes considered cleaner to always define functions before their use.

Some old comments still apply (magic number -6 and some namings), but there may be reasons for not changing that.

Code now almost reached the top of possibilities for non-object-oriented methodology, so I do not think it's worth crossing the line unless desired.

Using module-level or global data is usually frowned upon, because it makes functions to work magically (there are some exceptions of course). In case of module level configuration, one usually need to override the variable (eg, via monkeypatching). It's much easier for the function to accept the context, which may contain configuration. In the code above, load_survey_responses is the only violation I see, again, maybe it is justified. To see specifically what I mean, imaging that you will need to provide fp parameter as a script parameter: it will require changes to the code in several places instead of just providing fp upfront and propagating to further function calls as part of a context.

(Some readers may notice that prepare_survey_update now reminds Transaction Script pattern, see https://stackoverflow.com/questions/16139941/transaction-script-is-antipattern discussion on it's pros and cons)

Now that code became cleaner, I've noticed the script reads and writes to the same file (now it overwrites the file with each call). It's ok as far as you do not do things concurrently. But it's another story.

\$\endgroup\$
2
  • \$\begingroup\$ Imagining passing a custom fp made it clearer to understand. I've been wishy-washy with how I intended this program to run--I didn't decisively decide whether I wanted to allow the user to have control over the filepath or not. I do intend the module-level JSON data to remain in the same place, so I will update my code accordingly. I sincerely appreciate the code review. Thank you. \$\endgroup\$
    – Homer
    Commented Oct 10, 2017 at 11:00
  • \$\begingroup\$ Sure, those heuristics are not cut in stone. You know your case best. \$\endgroup\$
    – Roman Susi
    Commented Oct 10, 2017 at 12:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.