3
\$\begingroup\$

I have created a function that parses a directory tree, and returns a list of lists that groups specific files within for later program specific parsing. I feel like it could be done better, considering I've used dummy variables, and was hoping some of you could help.

def get_output_files(path) -> list:
    """
    This routine crawls through a directory tree, and checks for groups of job files, and groups based on:
        .sh   - run script file
        .hin  - hamiltonian creation input file
        .in   - iterate input file
        .hout - hamiltonian creation output file
        .out  - iterate output file
    to be used for parsing.

    Checks for grouping based on base name of file, if file suffix does not exist, then returns None in place.
    Example:  directory "/home/test" contains the following files:
        run.sh
        run.hin
        run2.in
        run.hout
        run2.out

    will return the following list:
        [
        [run.sh, run.hin, None, run.hout, None],
        [None, None, run2.in, None, run2.out]
        ]
    """
    jobs = []
    suffixes = ['.sh', '.hin', '.in', '.hout', '.out']
    for root, dirs, files in os.walk(path):
        for file in files:
            base, end = file.rsplit('.', 1)
            dummy_list = []
            for suffix in suffixes:
                if base + suffix in files:
                    dummy_list.append(os.path.join(root, base + suffix))
                else:
                    dummy_list.append(None)
            jobs.append(dummy_list)
    real_jobs = []
    for num, job in enumerate(jobs):
        if not all(element is None for element in job):
            real_jobs.append(job)
    return jobs
\$\endgroup\$
1
1
\$\begingroup\$

Don't worry about dummy variable names, naming is hard, and it can be fixed up later.

For my solution, there are a number of changes:

  • code is simplified by having a short inner function

  • the "yield" operator produces results one at a time, vs a list of results being processed in batch

  • the original code would process job files multiple times. IE if beer.sh and beer.hin were in a directory, the job would be scanned twice.

  • a simple test is added; modules added in PEP8 style; documentation; name clarification

source: mglob.py with test

'''
mglob.py -- print job files in directory

TEST USAGE:
    python -m unittest mglob
'''

import glob
import os
import unittest

def find_jobs(path):
    def find_job_paths(root, files, base):
        """
        given a job base filename, list related job files that exist
        """
        JOB_SUFFIXES = ['.sh', '.hin', '.in', '.hout', '.out']
        for suffix in JOB_SUFFIXES:
            newfile = base + suffix
            if newfile in files:
                yield os.path.join(root, newfile)
            else:
                yield None

    for root, dirs, files in os.walk(path):
        # process each file base name once
        bases = set(os.path.splitext(file_)[0]
                    for file_ in files)
        for base in bases:
            job_paths = list(find_job_paths(root, files, base))
            if any(job_paths):
                yield job_paths

class TestFind(unittest.TestCase):
    def test_simple(self):
        result = list(find_jobs('temp'))
        self.assertEquals(
            result, [
                [None, None, 'temp/run2.in', None, 'temp/run2.out'],
                ['temp/run.sh', 'temp/run.hin', None, 'temp/run.hout', None],
            ],
            result)
\$\endgroup\$
1
  • \$\begingroup\$ The multiple processing of files is something I was considering, and was planning on trying to fix, or hoping someone would see and point out. Your solution is what I was looking for. The addition of test cases and more simplicity also is what I was looking for. Thank you. \$\endgroup\$ – Corey Petty May 13 '16 at 19:27
0
\$\begingroup\$

Well one way I see you could be more Pythonic is by changing the line where you have

for suffix in suffixes:
    # ....

You already split the file and stored the file type in the variable 'end' Therefore all you need to do is to check if the value stored in end is in the suffixes list

if end in suffixes:
    # ....
else:
    # ....

And another thing, I don't see where real_jobs is being useful. You only return jobs and you didn't modify it in the last few lines of code. So, unless you have a typo or something, the last set of iteration is a waste of CPU cycles

\$\endgroup\$
1
  • \$\begingroup\$ The example case doesn't show when there are other types of files outside the defined suffixes, which is a case. I want to know if a given base can be grouped only within the suffixes. Maybe a case could be made if end is not in suffixes, then pass, which should skip unwanted filetypes. \$\endgroup\$ – Corey Petty May 13 '16 at 19:10
0
\$\begingroup\$

It looks good. I see one unnecessary call to enumerate in your last for loop as your are not using the array index num in that logic. (and as Michael mentioned, this loop didn't appear to be used)

\$\endgroup\$
0

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.