6
\$\begingroup\$

I have a piece of code that searches for XMLs in given folders, delete the ones older than X, zip the ones older than Y and delete zips older than X. It works perfectly but I have a terrible feeling that this is very inefficient code.

A little more information: It reads a config file for the times to delete/zip using Simple Config Parser, a library.

I am a temp at a networking company and this is one of the first pieces of "useful" code I've written.

import os, fnmatch, time, datetime, shutil, re
from lib.configobj import ConfigObj

if os.path.isfile('Cleaner_Script_Config.cfg')== True: 
    configreader = ConfigObj('Cleaner_Script_Config.cfg')
    How_Old_To_Delete_In_Days = configreader.get('How_Old_To_Delete_In_Days')
    How_Old_To_Zip_In_Days = configreader.get('How_Old_To_Zip_In_Days')
    Folders_To_Clean = os.path.join(configreader.get('Folders_To_Clean'))
    How_Old_To_Delete_In_Seconds = int(How_Old_To_Delete_In_Days)*24*60*60
    How_Old_To_Zip_In_Seconds = int(How_Old_To_Zip_In_Days)*24*60*60
    Folders_To_Clean_List= Folders_To_Clean.split(' ')
else:
    How_Old_To_Delete_In_Days = int('90')
    How_Old_To_Zip_In_Days = int('7')
    How_Old_To_Delete_In_Seconds = int(How_Old_To_Delete_In_Days)*24*60*60
    How_Old_To_Zip_In_Seconds = int(How_Old_To_Zip_In_Days)*24*60*60
    Folders_To_Clean = '/home/test/betfair-ftpdata /home/test/betfair-ftpdata2'
    Folders_To_Clean_List= Folders_To_Clean.split(' ')

def XML_Cleaner(extension, folders):
    for folder in folders:
        print('Running XML cleaner function')
        Files_To_Clean = []
        To_Delete_Time = (int(time.time()) - int(How_Old_To_Delete_In_Seconds))
        To_Zip_Time = (int(time.time()) - int(How_Old_To_Zip_In_Seconds))
        print ('All files older than: %s are to be deleted' % datetime.datetime.fromtimestamp(int(To_Delete_Time)).strftime('%Y-%m-%d %H:%M:%S'))
        print ('All files older than: %s are to be ziped' % datetime.datetime.fromtimestamp(int(To_Zip_Time)).strftime('%Y-%m-%d %H:%M:%S'))
        for root, dirs, files in os.walk(folder):
            for name in files:
                if fnmatch.fnmatch(name, extension):
                    Files_To_Clean.append(os.path.join(root, name))
        for file in Files_To_Clean:
            info = os.stat(file)
            Day_Folder = datetime.datetime.fromtimestamp(info.st_mtime).strftime('%Y-%m-%d')
            if not os.path.exists(Day_Folder):
                os.makedirs(Day_Folder)
            if (info.st_mtime < To_Delete_Time and info.st_mtime < To_Zip_Time):
                print ('File %s is to delete!' % file)
                os.remove(file)
            if (info.st_mtime < To_Zip_Time and info.st_mtime > To_Delete_Time):
                print ('File %s is to zip!' % file)
                infolder = Day_Folder + str(os.sep) + str(os.path.basename(os.path.normpath(file)))
                os.rename(file, infolder)
        List_of_Folders_Made = []
        reobj = re.compile('\d{4}-\d{2}-\d{2}')
        for root, subs, files in os.walk('.'):
            for name in subs:
                if reobj.match(name):
                    List_of_Folders_Made.append(name)
        for  madefolder in List_of_Folders_Made:
            shutil.make_archive(madefolder, 'zip', madefolder)
            shutil.rmtree(madefolder)
            os.rename(str(madefolder) + '.zip', str(folder) + str(os.sep) + str(madefolder) + '.zip')

def ZIP_Cleaner(extension, folders):
    for folder in folders:
        print('Running ZIP cleaner function')
        Files_To_Clean = []
        To_Delete_Time = (int(time.time()) - int(How_Old_To_Delete_In_Seconds))
        print ('All ZIP files older than %s will be deleted' % datetime.datetime.fromtimestamp(int(To_Delete_Time)).strftime('%Y-%m-%d %H:%M:%S'))
        for root, dirs, files in os.walk(folder):
            for name in files:
                if fnmatch.fnmatch(name, extension):
                    Files_To_Clean.append(os.path.join(root, name))
        for file in Files_To_Clean:
            info = os.stat(file)
            if (info.st_mtime < To_Delete_Time):
                print ('File %s is to delete!' % file)
                os.remove(file)

XML_Cleaner('*.XML', Folders_To_Clean_List)
XML_Cleaner('*.xml', Folders_To_Clean_List)
ZIP_Cleaner('*.zip', Folders_To_Clean_List)
\$\endgroup\$
4
\$\begingroup\$

Style


PEP8

PEP8 is python style guide. It says how you should style python code.

  • Lines should be a maximum of 79 characters.
    The exception to this is comments at 72.

  • Variables should be in snake_case. Not

    Capitalized_Words_With_Underscores (ugly!)

    The exception to this is classes with CamelCase.
    And Constants with UPPER_SNAKE_CASE.

  • Imports should be on separate lines.

  • Operators should have one space either side of them. 1 + 1.
    The exception to this is to show precedence. 2*2 + 1.

  • When comparing to singletons, True; None, use this is keyword.

  • When checking if something is true, don't compare to True.

  • Do not overwrite builtins, file, use a synonym, path, or add a trailing underscore, file_. The former is better.

You have a good style otherwise. It's nice seeing long variable names, as they are much better than a.


Code


String formatting

Currently you use both the + and % operators to add data to strings. You should use str.format instead.

'File %s is to delete!' % file
# To
'File {} is to delete!'.format(file)

os.rename(
     str(madefolder) + '.zip',
     str(folder) + str(os.sep) + str(madefolder) + '.zip')
# To
os.rename(
     '{!s}.zip'.format(makefolder),
     '{!s}{!s}{!s}.zip'.format(folder, os.sep, makefolder))

Getting the config

You may want to make things have absolute paths. If you call the file from a different directory, it may not work, as it will not be able to find the needed files.

This is as you may be in 'mydocuments', but the python file might be in 'programs'. The program will work relatively from 'mydocuments', and not 'programs'. And so you need to get a path for the settings.

file_path = os.path.dirname(__file__)

This with your os.path.isfile will mean that you will always open the correct settings.

if os.path.isfile(os.path.join(file_path, 'Cleaner_Script_Config.cfg')):
    # Do other stuff

In python it is very uncommon to do int('90'). Just do 90.


XML_cleaner

You call XML_cleaner twice, with the 'same' extension. This makes it so that you have to os.walk more than needed. I would change it so that you have os.walk as a separate function. Then you can pass in extensions, and how to handle them.

def walk_through_folders(folders, handlers):
    for folder in folders:
        for root, dirs, files in os.walk(folder):
            for name in files:
                extension = os.path.splitext(name)[1].lower()
                if extension in handlers:
                    handlers[extension](os.path.join(root, name))

walk_through_folders(folders, {'.xml': XML_cleaner, '.zip': ZIP_cleaner})

The way this will work is rather than handling everything for later, you handle it now. This has the pro of not building another list, which can be expensive later. It also means that you can handle ZIP files too. However it means that you don't build a list of paths.


Next we need to make it so there is a time that you will delete from. In the global scope you can add this.

This will make it so there is only one time that it will delete from. And will report it once.

def datetime_strftime(time, format_):
    return datetime.datetime.fromtimestamp(time).strftime(format_)

TO_DELETE_TIME = time.time() - How_Old_To_Delete_In_Seconds
TO_ZIP_TIME = time.time() - How_Old_To_Zip_In_Seconds
print('All files older than: {} are to be deleted.'.format(
    datetime_strftime(to_delete_time, '%Y-%m-%d %H:%M:%S')
))
print('All files older than: {} are to be ziped.'.format(
    datetime_strftime(to_zip_time, '%Y-%m-%d %H:%M:%S')
))

As we are handling each file one at a time we will have to split the remaining of XML_cleaner into two functions. One that cleans up the zip files, and another that will handle the single path passed to it.

This means that after we have completed walk_through_folders, we will need to call zip_files().

def XML_cleaner(path):
    info = os.stat(path)
    day_folder = datetime.datetime.fromtimestamp(info.st_mtime) \ 
                 .strftime('%Y-%m-%d')
    if not os.path.exists(day_folder):
        os.makedirs(day_folder)
    if info.st_mtime < TO_DELETE_TIME:
        print ('Deleting: {!r}'.format(path))
        os.remove(path)
    elif info.st_mtime < TO_ZIP_TIME:
        print ('Ziping: {!r}'.format(path))
        infolder = (day_folder +
                    os.sep +
                    os.path.basename(os.path.normpath(path)))
        os.rename(path, infolder)

def zip_folders(folders):
    reobj = re.compile('\d{4}-\d{2}-\d{2}')
    for folder in folders:
        for root, subs, files in os.walk('.'):
            for name in subs:
                if reobj.match(name):
                    shutil.make_archive(name, 'zip', name)
                    shutil.rmtree(name)
                    os.rename(
                        '{!s}.zip'.format(name),
                        '{!s}{!s}{!s}.zip'.format(folder, os.sep, name))

ZIP_cleaner

With the changes to XML_cleaner we can make a small edit to ZIP_cleaner. As we are using walk_through_folders to loop, we need ZIP_cleaner to just handle one path as input.

def ZIP_cleaner(path):
    info = os.stat(path)
    if info.st_mtime < TO_DELETE_TIME:
        print ('Deleting: {!r}'.format(path))
        os.remove(path)

Combine all changes.

As most of it is modular, we will just be changing the global scope. We will move everything together, wrap it in a __name__=='__main__', and then remove duplicates in this.

# At the bottom of the file.
if __name__=='__main__':
    file_path = os.path.dirname(__file__)
    if os.path.isfile(os.path.join(file_path, 'Cleaner_Script_Config.cfg')): 
        configreader = ConfigObj('Cleaner_Script_Config.cfg')
        how_old_to_delete = configreader.get('How_Old_To_Delete_In_Days')
        how_old_to_zip = configreader.get('How_Old_To_Zip_In_Days')
        folders = os.path.join(configreader.get('Folders_To_Clean'))
    else:
        how_old_to_delete = 90
        how_old_to_zip = 7
        folders = '/home/test/betfair-ftpdata ' \
                  '/home/test/betfair-ftpdata2'
    TO_DELETE_TIME = time.time() - (how_old_to_delete * 24 * 60 * 60)
    TO_ZIP_TIME = time.time() - (how_old_to_zip * 24 * 60 * 60)

    print('All files older than: {} are to be deleted.'.format(
        datetime_strftime(TO_DELETE_TIME, '%Y-%m-%d %H:%M:%S')
    ))
    print('All files older than: {} are to be ziped.'.format(
        datetime_strftime(TO_ZIP_TIME, '%Y-%m-%d %H:%M:%S')
    ))

    walk_through_folders(
         folders.split(' '),
         {'.xml': XML_cleaner, '.zip': ZIP_cleaner})
    zip_folders(folders.split(' '))

Conclusion

You os.walk(folder) three times. This can be limited to once with the changes above. This means that you take three times longer then it should.
There is also a 'problem' with array.append where when the list is very large it takes a long time to append.

You had a lot of duplicate logic, now reduced to XML_cleaner and ZIP_cleaner. If you had wanted to add another file type you would have had to copy and paste a lot of the same stuff. And it is a pain to fix bugs when you have lots of functions that do the same thing most of the time.
Now you have that limited to just how you deal with the path, and not how you get to the path and how to deal with it.

You could probably change the zip_files function so that it works in the XML_cleaner function. However personally I think, even though it's more taxing on the hard-drives, this is a nicer way to do it.
You can then just add more file-types to become zips, and it will do it all at the end.

You could probably have zip_files in walk_through_folders. The reason I didn't in the fix was to keep the logic mostly the same, and because you walk through different places, os.walk(folder) and os.walk('.'). It will also have no difference on the hard-drives.

You can change the if __name__=='__main__' so that it just calls a main().
This is good, as then there will be less things in the global scope.
However you would then have to add TO_DELETE_TIME and TO_ZIP_TIME to the global scope another way.

I also only changed one function, fnmatch.fnmatch(name, extension), to, os.path.splitext(name)[1].lower(), this is so that you can compare it's extension to the dictionary. I have not tested if it is the same, but it should be.

\$\endgroup\$
  • \$\begingroup\$ I will test it in a moment and if it works I will definately accept, thank you for the extensive anwser and tips! Hopefully my next project will have better logic and less repetition. \$\endgroup\$ – Seraphim Jul 15 '15 at 13:09
  • \$\begingroup\$ Unfortuanately I am currently working on something else and cannot check but I promise to check and accept your anwser tomorrow at the latest (Is that ok?) \$\endgroup\$ – Seraphim Jul 15 '15 at 13:32
  • \$\begingroup\$ There were a few errors in syntax but works after correcting. Thank you very much \$\endgroup\$ – Seraphim Jul 16 '15 at 16:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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