5
\$\begingroup\$

This is my first attempt at writing code that moves files at the end of the day, from my local work folder to an network archive.

The program works but, any tips, tricks, or ideas to make this code better would be most appreciated.

import shutil, os, datetime, time, sys

os.system('TASKKILL /F /IM FlashRen.exe') 

newFolderName = datetime.datetime.now()
newFolder = newFolderName.strftime('%m%d%Y')


src = ('C:\\AEC Work\\' + newFolder)
dst = ('\\\qnap01\\Data Service\\AARON\\' +
           newFolderName.strftime('%Y\\') +
           newFolderName.strftime('%m. %B\\').upper() + newFolder)

for file in os.listdir(src):
    src_file = os.path.join(src, file)
    dst_file = os.path.join(dst, file)
    shutil.move(src, dst)
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Doing multiple imports on the same line from different modules is frowned upon by PEP8, Python's official style-guide.

It also recommends using lower_case for variable names and not camelCase.

You don't need () around variables when assigning, src = 'C:\AEC Work\' + newFolder is enough.

Since you are already using os.path.join, which is a good thing, you should use it throughout.

You can use str.format to get "{:%m%d%Y}".format(datetime.datetime.now()) for the new folder name to make this slightly more compact.

import shutil
import os
import datetime
import time
import sys

os.system('TASKKILL /F /IM FlashRen.exe') 

now = datetime.datetime.now()
directory = "{:%m%d%Y}".format(now)

src = os.path.join('C:','AEC Work', directory)
backup_base = os.path.join('\qnap01', 'Data Service', 'AARON')
dst = os.path.join(backup_base, "{:%Y}".format(now),
                   "{:%m. %B}".format(now).upper(), directory)

for file in os.listdir(src):
    src_file = os.path.join(src, file)
    dst_file = os.path.join(dst, file)
    shutil.move(src, dst)
\$\endgroup\$
1
\$\begingroup\$

Adding to Graipher's excellent answer, I'd wrap everything inside some functions and call that from __main__. This way, you'll keep things even more obvious than they are now.

More, in the last 2/3 last lines, didn't you mean:

shutil.move(src_file, dst_file)

?

Anyhow, there's how I'd further improve it:

import shutil
import os
import datetime


def kill_flashren_proc():
    os.system('TASKKILL /F /IM FlashRen.exe')


def get_current_time():
    return datetime.datetime.now()


def create_directory_name():
    return "{:%m%d%Y}".format(get_current_time())


def main():
    kill_flashren_proc()

    now = get_current_time()
    directory = create_directory_name()

    src = os.path.join('C:', 'AEC Work', directory)
    backup_base = os.path.join('\qnap01', 'Data Service', 'AARON')
    dst = os.path.join(backup_base, "{:%Y}".format(now), "{:%m. %B}".format(now).upper(), directory)

    for file in os.listdir(src):
        src_file = os.path.join(src, file)
        dst_file = os.path.join(dst, file)
        shutil.move(src_file, dst_file)

if __name__ == '__main__':
    main()

Note that you also don't need time and sys modules anymore so you can take them out.

\$\endgroup\$

Your Answer

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

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