5
\$\begingroup\$

This script creates a hierarchy of directories. Is this a good approach or can it be made better? I am mainly concerned with maintainability here, not efficiency. Suppose I wanted to add more directories to this hierarchy. Would this structure of the script make it easier to do?

Hierarchy

./project_euler
    ./001_050
    ./051_100
    ...
    ./401_450
./codechef
    ./easy
    ./medium
    ./hard
./spoj
./functions
./utilities

The script

import os

TOP = ['project_euler', 'codechef', 'spoj', 'functions',
       'utilities']
CODECHEF = ['easy', 'medium', 'hard']

def safe_make_folder(i):
    '''Makes a folder if not present'''
    try:  
        os.mkdir(i)
    except:
        pass

def make_top_level(top):
    for i in top:
        safe_make_folder(i)

def make_euler_folders(highest):
    def folder_names():
        '''Generates strings of the format 001_050 with
        the 2nd number given by the function argument'''
        for i in range(1,highest, 50):
            yield (
                'project_euler' + os.sep +
                str(i).zfill(3) + '_' + str(i + 49).zfill(3)
                )

    for i in folder_names():
        safe_make_folder(i)

def make_codechef_folders(codechef):
    for i in codechef:
        safe_make_folder('codechef' + os.sep + i)

def main():
    make_top_level(TOP)
    make_euler_folders(450)
    make_codechef_folders(CODECHEF)

if __name__ == "__main__":
    main()
\$\endgroup\$
8
  • 1
    \$\begingroup\$ PEP 8 advices lower case for local variables. \$\endgroup\$ – SylvainD Jul 19 '13 at 19:58
  • 1
    \$\begingroup\$ @Josay HIGHEST is constant, but not defined at the module level. PEP8: Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL. \$\endgroup\$ – Anthon Jul 20 '13 at 16:37
  • \$\begingroup\$ @Lstor said that global variables are code-smell so I thought to make them local variables(which are constants so I used capital letters). What would be the correct design decision here? \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 16:59
  • \$\begingroup\$ @Josay Please see the above \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 16:59
  • \$\begingroup\$ @Anthon Please see the above comment. \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 17:08
4
\$\begingroup\$

One of the things I would do is remove the double occurrence of the strings 'project_euler' and 'codechef'. If you ever have to change one of these in TOP, you are bound to miss the repetition in the functions.

You should at least use TOP[0] in make_euler_folders() and TOP[1] in make_codechef_folders. A better approach however would be to take both definitions out of TOP and change def safe_make_folder():

TOP = ['spoj', 'functions', 'utilities']

def safe_make_folder(i):
    '''Makes a folder (and its parents) if not present'''
    try:  
        os.makedirs(i)
    except:
        pass

The standard function os.makedirs() creates the 'project_euler' resp. 'codechef', as the first subdirectory of each is created.


The other is that I would create the directory names using os.path.join() (as it prevents e.g. the mistake of providing double path separators), in combination with standard string formatting to get the leading zeros on the subfolder names:

os.path.join('project_euler', '{:03}_{:03}'.format(i, i+49))

the {:03} gives a 3 character wide field with leading zeros. @Josay improvemed function would then become:

def make_euler_folders(highest):
    for i in (os.path.join('project_euler', '{:03}_{:03}'.format(i, i+49)) \
       for i in range(1,highest, 50)):
        safe_make_folder(i)

And the other function would become:

def make_codechef_folders(codechef):
    for i in codechef:
        safe_make_folder(os.path.join('codechef', i))
\$\endgroup\$
2
  • \$\begingroup\$ Updated the code. Any other suggestions? \$\endgroup\$ – Aseem Bansal Jul 19 '13 at 16:40
  • \$\begingroup\$ move the constant HIGHEST to the module level and make it a somewhat more descriptive name (HIGHEST_EXERSIZE_NUMBER ?) \$\endgroup\$ – Anthon Jul 20 '13 at 16:38
2
\$\begingroup\$

I'm not quite convinced that a python script is required here as a shell one-liner would probably do the trick (mkdir -p codechef/{easy,medium,hard} spoj utilities would be a good starting point).

Your python code could be improved by using Generator Expressions :

def make_euler_folders(highest):
    def folder_names():
        '''Generates strings of the format 001_050 with
        the 2nd number given by the function argument'''
        for i in range(1,highest, 50):
            yield (
                'project_euler' + os.sep +
                str(i).zfill(3) + '_' + str(i + 49).zfill(3)
                )

    for i in folder_names():
        safe_make_folder(i)

would become

def make_euler_folders(highest):
    def folder_names():
        '''Generates strings of the format 001_050 with
        the 2nd number given by the function argument'''
        return ('project_euler' + os.sep + str(i).zfill(3) + '_' + str(i + 49).zfill(3) for i in range(1,highest, 50))

    for i in folder_names():
        safe_make_folder(i)

which is then nothing but :

def make_euler_folders(highest):
    for i in ('project_euler' + os.sep + str(i).zfill(3) + '_' + str(i + 49).zfill(3) for i in range(1,highest, 50)):
        safe_make_folder(i)
\$\endgroup\$
5
  • \$\begingroup\$ I used Python because I am on Windows. \$\endgroup\$ – Aseem Bansal Jul 19 '13 at 14:57
  • \$\begingroup\$ Anything about use of global variables? Should I pass them or not? Best practice about that? \$\endgroup\$ – Aseem Bansal Jul 19 '13 at 15:02
  • 1
    \$\begingroup\$ You should pass them. Global variables is a code smell, and leads to tighter coupling (which is bad). \$\endgroup\$ – Lstor Jul 19 '13 at 15:25
  • 1
    \$\begingroup\$ Also, you should probably check your usage of literal strings as some of them are defined twice ('project_euler', 'codechef'). \$\endgroup\$ – SylvainD Jul 19 '13 at 15:50
  • \$\begingroup\$ @Josay Is it better now? \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 5:46

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.