2hands10fingers / Reddit-Image-Scraper-1.0

Scrapes/downloads a selected subreddit's posted images by a specified date range on http://reddit.com
http://www.glotacosm.com
48 stars 13 forks source link

Code cleanup #12

Closed AndreiUngur closed 6 years ago

AndreiUngur commented 6 years ago

Made the code more modular by placing many chunks into functions, to make the logic of the input handling part easier to follow. I've tested this by running it locally and everything worked as it did before. No changes in functionality.

tjcim commented 6 years ago

Hey, good work. I am afraid to work on it anymore until I know how the PRs are going to merge.... In the meantime I have a suggestion. Why not abstract the questions and the validators? For example, instead of this:

def validateStartYear(year_b):
    while not year_b in range(2005, int(datetime.now().year +1)):
        print("\n\tWoops! Try entering a valid number. " + str(year_b) + " was not a valid number.")
        time.sleep(2)
        time_clear_header1()
        year_b = int(input('\n\tEnter the year you would like to START your range: \n\t'))
    return year_b

We do this:

def valid_year(year):
    return year in range(2005, int(datetime.now().year +1))

def prompt(question, error, validator):
    results = int(input(prompt))
    while not validator(results):
        print(error)
        time.sleep(2)
        time_clear_header1()
        results = int(input(prompt))
    return results

begin_year = prompt("\n\tEnter the year you would like to START your range: \n\t", "\n\tWoops! Try entering a valid number. " + str(year_b) + " was not a valid number.", valid_year)

That way we only have to write the prompt logic once and call it as needed.

One last thing... why are we doing + 1? It seems that 2017 would be the highest number we would want to include, or am I missing something?

AndreiUngur commented 6 years ago

Hi, I've applied the fixes you mentioned, the code does look a lot better now ! I didn't just abstract "validateStartYear" into "validate_year", but I actually made all 6 of my previous validators into 1.

I also added a section for constants to make it easier to edit the messages. I didn't want to put all the messages in there for now, I only put the ones affected by my changes. It also has the valid ranges for years/months.

I also wrote all my functions in snake case to follow the rest of the code, and I removed the "+1" which is indeed useless!

tjcim commented 6 years ago

Nice job!

def validate_date(date,range,message):
    while not valid(date,range):

Because date and range are special words in python, we should really use another name for the variables.

While we are working on that one, we can shorten these lines into one:

year_b = int(input(year_start_msg))
year_b = validate_date(year_b, year_range, year_start_msg)

By doing this:

def get_valid_date(valid_range, message):
    user_input = int(input(message))
    while not valid(user_input, valid_range):
        print("\n\tWoops! Try entering a valid number. " + str(user_input) + " was not a valid number.")
        time.sleep(2)
        time_clear_header1()
        user_input = int(input(message))
    return date

year_b = get_valid_date(year_range, year_start_msg)
month_b = get_valid_date(month_range, month_msg)
day_b = get_valid_date(set_date_range(month_b), day_msg) 

One other thing that I was wondering, why is the code using Delorean? It seems silly to require the Delorean package when we don't really need it. For instance we could do something like this:

epoch = datetime.datetime(1970, 1, 1)
bd_epoch = (datetime.datetime(year_b, month_b, day_b) - epoch).totalseconds()
AndreiUngur commented 6 years ago

Sorry about that, missed those ! I'm also not sure if the use of Delorean is justified, but it seems that it handles time zones so maybe it was added to eventually account for those?

Side note: I added the "+1" back to the "current date" for the year range because I noticed that removing it made 2017 become an illegal year. This is because python's range goes up to (without including) the last value. This means that there was an issue with how the range for days was handled also (range[0,30] excludes 30, which is intended to be a valid day).

tjcim commented 6 years ago

Awesome! Thank you.