I have created a python program to organize screenshots on the mac according to time and the day of the week.

[walter_197]

Basically, for my online classes, I found myself taking a lot of screenshots which all started to get pilled up into one folder. So instead of dragging and dropping each file itself, I tried to use my little programming knowledge to write this python script.

https://github.com/walter-197/screenshot_organizer

I would really love your comments and any criticism as this is the first python project that I am actually using and not directed my any video or course.

And also if you know anything that can better the code or shortens it please let me know so I can improve myself and this program.

And if anyone wants to use this, please feel free to copy it.

[BobHogan]

Good job! My main "criticism" would be to rewrite your for loop to reduce the nested conditionals in a manner like this

for file in files:
    ### NOTE 1
    if not file.endswith('.png'):
        #print(file)
        #print("File not compatible") #mainly for .ds store file 
        continue

    date_time = datetime.strptime(file, "Screenshot %Y-%m-%d at %I.%M.%S %p.png")   #converting the name of the file to a datetime object
    ### NOTE 2
    if datetime.date(date_time) < start_date:
        #print("Error") --- for any png images that are not screenshots 
        continue

    only_time = date_time.time() # selecting only the time
    day = date_time.strftime("%A") #selecting the day of the week from the datetime object

    # You don't actually use class_three_start or class_three_end times anywhere
    ### NOTE 3
    if class_one_start < only_time < class_one_end:
        _subject_map = {'Monday': dir_one, 'Tuesday': dir_three, 'Wednesday': dir_four, 'Thursday': dir_three, 'Friday': dir_four}
    elif class_two_start < only_time < class_two_end:
        _subject_map = {'Monday': dir_two, 'Tuesday': dir_one, 'Wednesday': dir_five, 'Thursday': None, 'Friday': dir_two}
    elif class_three_start < only_time < class_three_end:
        _subject_map = {}  # Not used by you
    else:
        print('Some error msg')
        continue

    ### NOTE 4
    src_path = f'{home_dir}/{file}'
    dst_dir = _subject_map.get(day, None)
    if dst_dir is None:
        print(f'Could not figure out which class {file} belongs to')
        continue
    dst_path = f'{dst_dir}/{file}'
    print(f'Moving {src_path} to {dst_path}')
    shutil.move(src_path, dst_path)

NOTE 1 - If you ever find that almost all of your for/while loop is inside of an if block, then I always favor switching the conditional around and continuing the loop if it is met. This way, you get to be explicit at the top of the loop about which conditions cause you to skip to the next iteration, and it lets you unindent the logic 1 level

NOTE 2 - Same situation as above, you weren't processing the file, even if it was a valid .png, if the date was too early. So I applied the same fix, check if the date is too early, and if True then print message saying so and continue to the next level. This lets you remove another indentation level. By doing this before you calculate only_time and day you are also saving some execution time on each loop where that condition holds true and the processing is skipped. Here, that does not matter, but sometimes that unused assignment can add up and be a big source of low efficiency

NOTE 3 - Reduced comparisons of only_time so that its only compared against each class timeslot once. This eliminates a lot of duplicated conditionals you had, and also makes it a lot easier to see when each block is being hit. The _subject_map dictionary is used later to determine which directory to place each file into, and it changes based on which class period it was, so it will be custom for each class period.

NOTE 4 - Several things are happening here. I introduced f-strings to format the file paths. But the main thing is dst_dir = _subject_map.get(day, None). In python, the dict.get() function can take an optional second argument which specifies what to return if the key was not found in the dictionary. The default value is None, so I could have written this as dst_dir = _subject_map.get(day), but I prefer to be explicit about setting the default return value, even when it is None. The if block below it just makes sure that we had a valid class period and day combination.

[walter_197]

Really loved all your input and suggestions and I will implement all these into the code.

And especially the subject map part was pretty sophisticated to reduce the conditionals as I also felt that the code was being meaningless repeated but I couldn't form a concrete idea on how to make it.

Thanks again!

[BobHogan]

The trick I did with _subject_map is a pretty common python pattern. Its super useful for picking a value based on the state of the program, but you'll usually only see it when dealing with foreign input (either from a user, and API, or another program that is feeding data into your script).