1
\$\begingroup\$

I have a dict with a series of bools to specify if a particular aspect of my program will be run (e.g. settings['A'] and settings['B']).

If they are run the results need to go into an aptly named directory. The directory names (not the entire path) are also stored in the dict (e.g. settings['outputDirNameOut1'] and settings['outputDirNameA'])

In order to create the directories I'm using the following code.

for each in filePathList:
    path = os.path.dirname(each)

    if not os.path.exists(os.path.join(path, settings['outputDirNameOut1'])):
        os.makedirs(os.path.join(path, settings['outputDirNameOut1']))

    if settings['A'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameA'])):
        os.makedirs(os.path.join(path, settings['outputDirNameA']))

    if settings['B'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameB'])):
        os.makedirs(os.path.join(path, settings['outputDirNameThreshold']))

    if settings['C'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameC'])):
        os.makedirs(os.path.join(path, settings['outputDirNameC']))
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

    if settings['D'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameOut2'])):
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

I'm wondering if there's a simpler, easier to read way to accomplish this.

\$\endgroup\$
2
\$\begingroup\$

Any time there's a repeating pattern that's very obvious like this you might consider using another loop, or a function, or some other abstraction mechanism available. To me this also looks like example code - please post the actual code you're using!

So first a few comments:

  • x is True isn't very idiomatic unless you really want to check for specifically one of the true values, True. Otherwise just write that as x (keeping in mind that some values like the empty tuple or list are actually falsy.

  • Some expressions are repeated in the if test and the body of the if - in those cases you likely want to have a variable to only evaluate the expression once for better readability.

  • Consider using some functional idioms like map to make the code more succinct too.

  • Take a look at PEP8 for variable naming - in Python it's usually snake case.

So how with that said, how could this all look like?

for path in map(os.path.dirname, file_path_list):
    x = os.path.join(path, settings['outputDirNameOut1'])
    if not os.path.exists(x):
        os.makedirs(x)

    y = os.path.join(path, settings['outputDirNameA'])
    if settings['A'] and not os.path.exists(y):
        os.makedirs(y)

    z = os.path.join(path, settings['outputDirNameB'])
    if settings['B'] and not os.path.exists(z):
        os.makedirs(z)

    a = os.path.join(path, settings['outputDirNameC'])
    if settings['C'] and not os.path.exists(a):
        os.makedirs(a)
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

    b = os.path.join(path, settings['outputDirNameOut2'])
    if settings['D'] and not os.path.exists(b):
        os.makedirs(b)

Now you just have to come up with more sensible variable names. Also I have the feeling that the double makedirs in the fourth case is a copy&paste error?

Going with that assumption this can be simplified further:

names = ['outputDirNameOut1', 'outputDirNameA', 'outputDirNameB',
         'outputDirNameC', 'outputDirNameOut2']
dirs = [settings[name] for name in names]
for path in map(os.path.dirname, file_path_list):
    for dir in dirs:
        x = os.path.join(path, dir)
        if not os.path.exists(x):
            os.makedirs(x)

At that point I can't see any more obvious changes.


Edit: That assumption doesn't hold and I also skipped some part of the logic. Therefore I'd suggest writing and using a local function:

def foo(name):
    x = os.path.join(path, settings[name])
    if not os.path.exists(x):
        os.makedirs(x)

for path in map(os.path.dirname, file_path_list):
    foo('outputDirNameOut1')

    if settings['A']:
        foo('outputDirNameA')

    if settings['B']:
        foo('outputDirNameB')

    if settings['C']:
        foo('outputDirNameC')
        foo('outputDirNameOut2')

    if settings['D']:
        foo('outputDirNameOut2')
\$\endgroup\$
  • \$\begingroup\$ This is extremely helpful. A few questions. If there a way in the further simplification to integrate the check for the bool being true before creating the new directory? The program has a bunch of functions which are turned optionally turned on. The results of each function get stored in the new directories. Creating dirs for the output of functions that haven't been run is just confusing. The fourth case isn't a typo. There are 2 outputs from that function. The results from 5 go in the same dir as one of the results of 4 but the user as the option to run both, one, or neither. \$\endgroup\$ – agf1997 Jul 2 '18 at 23:35
  • \$\begingroup\$ @agf1997 I added a bit since I noticed I skipped some part of the logic too. Regarding your question, I'm not quite sure what you mean, if it was regarding the checks that I didn't notice before, see above, otherwise please post some actual code. \$\endgroup\$ – ferada Jul 3 '18 at 20:27

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.