0
\$\begingroup\$

I am trying to build a personal content aggregator and I started off with Reddit.
My aim is to build something that is designed properly for this task so that I could add more sources and delivery options easily.

If possible, I'd like you to focus your reviews on the design of the code (any other feedback is welcome as well)

from abc import ABC, abstractmethod
import praw
import os

CLIENT_ID = os.environ.get('REDDIT_CLIENT_ID')
CLIENT_SECRET = os.environ.get('REDDIT_CLIENT_SECRET')


class Source(ABC):
    
    @abstractmethod
    def connect(self):
        pass

    @abstractmethod
    def fetch(self):
        pass


class RedditSource(Source):

    def connect(self):
        self.reddit_con = praw.Reddit(client_id=CLIENT_ID,
                     client_secret=CLIENT_SECRET,
                     grant_type_access='client_credentials',
                     user_agent='script/1.0')
        return self.reddit_con

    def fetch(self):
        pass


class RedditHotProgramming(RedditSource):
    
    def __init__(self) -> None:
        self.reddit_con = super().connect()
        self.hot_submissions = []

    def fetch(self, limit: int):
        for submission in self.reddit_con.subreddit('programming').hot(limit=limit):
            self.hot_submissions.append(submission)

    def __repr__(self):
        urls = []
        for submission in self.hot_submissions:
            urls.append(vars(submission)['url'])
        return '\n'.join(urls)


if __name__ == '__main__':
    assert CLIENT_ID, CLIENT_SECRET
    reddit_top_programming = RedditHotProgramming()
    reddit_top_programming.fetch(limit=10)
    print(reddit_top_programming)

I am also pondering on how should I add a delivery module. as of now, my idea was to create a separate class with the strategy pattern in mind to allow different types of deliveries (email, IM, etc)

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Your class tree is kind of scrambled. It doesn't make sense to have an abstract connect - not all sources will need connection, and any connection can just happen in the constructor.

RedditSource.connect is confused because it both sets a member - when best practices discourage setting new members outside of __init__ - and returns it, only for the return value to be used in super().connect() - only to overwrite the same member variable. I would refactor this so that there is no connect method at all; and RedditSource.__init__ instantiates the praw.Reddit object.

Is the return value of hot() a source-agnostic type? I suspect not. It looks like your conversion from source-specific to source-agnostic data occurs in your __repr__. This is a problem because the type of hot_submissions will differ from the type of a similar member in a different source. One solution is

  • Don't have a hot_submissions member at all, making the source class a source only and not a repository of data;
  • Do not mutate self during fetch;
  • Yield submissions from your fetch; and
  • have the format of those submissions be whatever common format you care about from all of your sources, non-specific to Reddit. If you only care about a URL list, the return type of fetch should be an iterable of URLs with conversion happening in fetch or a subroutine.

Do not include assert in production code as it can be trivially disabled.

\$\endgroup\$

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.