Take the 2-minute tour ×

Hi I have the following procedure,

Questions: - How to make it elegant, more readable, compact. - What can I do to extract common loops to another method.


From a given rootDir the dirs are organized as in ex below.

What the proc does:

If input is 200, it deletes all DIRS that are OLDER than 200 days. NOT based on modifytime, but based on dir structure and dir name [I will later delete by brute force "rm -Rf" on each dir that are older]

e.g dir structure:

-2009(year dirs) [will force delete dirs e.g "rm -Rf" later]
  -01...(month dirs)
  -05 ..
      -01.. (day dirs)
         -many files. [I won't check mtime at file level - takes more time]
-2012 ...

Code that I have:

def get_dirs_to_remove(dir_path, olderThanDays):
    today = datetime.datetime.now();
    oldestDayToKeep = today + datetime.timedelta(days= -olderThanDays) 
    oldKeepYear = int(oldestDayToKeep.year)
    oldKeepMonth =int(oldestDayToKeep.month);
    oldKeepDay = int(oldestDayToKeep.day);
    for yearDir in os.listdir(dirRoot):
        #iterate year dir
        yrPath = os.path.join(dirRoot, yearDir);
        if(is_int(yearDir) == False):
            problemList.append(yrPath); # can't convery year to an int, store and report later 

        if(int(yearDir) < oldKeepYear):
                print "old Yr dir: " + yrPath
                #deleteList.append(yrPath); # to be bruteforce deleted e.g "rm -Rf"
                yield yrPath;
        elif(int(yearDir) == oldKeepYear):
            # iterate month dir
            print "process Yr dir: " + yrPath
            for monthDir in os.listdir(yrPath):
                monthPath = os.path.join(yrPath, monthDir)
                if(is_int(monthDir) == False):
                if(int(monthDir) < oldKeepMonth):
                        print "old month dir: " + monthPath
                        yield monthPath;
                elif (int(monthDir) == oldKeepMonth):
                    # iterate Day dir
                    print "process Month dir: " + monthPath
                    for dayDir in os.listdir(monthPath):
                        dayPath = os.path.join(monthPath, dayDir)
                        if(is_int(dayDir) == False):
                        if(int(dayDir) < oldKeepDay):
                            print "old day dir: " + dayPath
                            yield dayPath
print [ x for x in get_dirs_to_remove(dirRoot, olderThanDays)]
print "probList" %  problemList # how can I get this list also from the same proc?
share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

This actually looks pretty nice, except for the one big thing mentioned in this comment:


It sounds like you're storing problemList in a global variable or something, and you'd like to fix that. Here are a few ways to do this:

  • Yield both delete files and problem files—e.g., yield a tuple where the first member says which kind it is, and the second what to do with it.
  • Take the problemList as a parameter. Remember that lists are mutable, so appending to the argument will be visible to the caller.
  • yield the problemList at the end—which means you need to restructure the way you use the generator, because it's no longer just a simple iterator.
  • Code the generator as a class instead of a function, and store problemList as a member variable.
  • Peek at the internal generator information and cram problemList in there, so the caller can retrieve it.

Meanwhile, there are a few ways you could make the code more compact and readable.

Most trivially:


This list comprehension is exactly the same as the original iteration, which you can write more simply as:

print list(get_dirs_to_remove(dirRoot, olderThanDays))

As for the algorithm itself, you could partition the listdir, and then just use the partitioned lists. You could do it lazily:

yearDirs = os.listdir(dirRoot):
problemList.extend(yearDir for yearDir in yearDirs if not is_int(yearDir))
yield from (yearDir for yearDir in yearDirs if int(yearDir) < oldKeepYear)
for year in (yearDir for yearDir in yearDirs if int(yearDir) == oldKeepYear):
    # next level down

Or strictly:

yearDirs = os.listdir(dirRoot)
problems, older, eq, newer = partitionDirs(yearDirs, oldKeepYear)
yield from older
for year in eq:
    # next level down

The latter probably makes more sense, especially given that yearDirs is already a list, and isn't likely to be that big anyway.

Of course you need to write that partitionDirs function—but the nice thing is, you get to use it again in the months and days levels. And it's pretty simple. In fact, I might actually do the partitioning by sorting, because it makes the logic so obvious, even if it's more verbose:

def partitionDirs(dirs, keyvalue):
    problems = [dir for dir in dirs if not is_int(dir)]
    values = sorted(dir for dir in dirs if is_int(dir), key=int)
    older, eq, newer = partitionSortedListAt(values, keyvalue, key=int)

If you look around (maybe search "python partition sorted list"?), you can find lots of ways to implement the partitionSortedListAt function, but here's a sketch of something that I think is easy to understand for someone who hasn't thought of the problem this way:

    i = bisect.bisect_right(vals, keyvalue)
    if vals[i] == keyvalue:
        return problems, vals[:i], [vals[i]], vals[i+1:]
        return problems, vals[:i], [], vals[i:]

If you search for "python split predicate" you can also find other ways to implement the initial split—although keep in mind that most people are either concerned with being able to partition arbitrary iterables (which you don't need here), or, rightly or not, worried about efficiency (which you don't care about here either). So, don't look for the answer that someone says is "best"; look at all of the answers, and pick the one that seems most readable to you.

Finally, you may notice that you end up with three levels that look almost identical:

yearDirs = os.listdir(dirRoot)
yield from older
for year in eq:
    monthDirs = os.listdir(os.path.join(dirRoot, str(year)))
    problems, older, eq, newer = partitionDirs(monthDirs, oldKeepMonth)
    yield from older
    for month in eq:
        dayDirs = os.listdir(os.path.join(dirRoot, str(year), str(month)))
        problems, older, eq, newer = partitionDirs(dayDirs, oldKeepDay)
        yield from older
        yield from eq

You can simplify this further through recursion—pass down the path so far, and the list of further levels to check, and you can turn this 18 lines into 9. Whether that's more readable or not depends on how well you manage to encode the information to pass down and the appropriate yield from. Here's a sketch of the idea:

def doLevel(pathSoFar, dateComponentsLeft):
    if not dateComponentsLeft:
    dirs = os.listdir(pathSoFar)
    problems, older, eq, newer = partitionDirs(dirs, dateComponentsLeft[0])
    yield from older
    if eq:
        yield from doLevel(os.path.join(pathSoFar, eq[0]), dateComponentsLeft[1:]))
yield from doLevel(rootPath, [oldKeepYear, oldKeepMonth, oldKeepDay])

If you're on an older Python version that doesn't have yield from, the earlier stuff is almost trivial to transform; the recursive version as written will be uglier and more painful. But there's really no way to avoid this when dealing with recursive generators, because a sub-generator cannot "yield through" a calling generator.

share|improve this answer
add comment

I would suggest not using generators unless you are absolutely sure you need them. In this case, you don't need them.

In the below, newer_list isn't strictly needed. While categorizeSubdirs could be made recursive, I don't feel that the increase in complexity is worth the repetition savings (but that's just a personal style issue; I only use recursion when it's unclear how many levels of recursion are needed or the number is fixed but large; three isn't enough IMO).

def categorizeSubdirs(keep_int, base_path):
    older_list = []
    equal_list = []
    newer_list = []
    problem_list = []

    for subdir_str in os.listdir(base_path):
        subdir_path = os.path.join(base_path, subdir_str))
            subdir_int = int(subdir_path)
        except ValueError:
            if subdir_int  keep_int:

    # Note that for your case, you don't need newer_list, 
    # and it's not clear if you need problem_list
    return older_list, equal_list, newer_list, problem_list

def get_dirs_to_remove(dir_path, olderThanDays):
    oldest_dt = datetime.datetime.now() datetime.timedelta(days= -olderThanDays) 
    remove_list = []
    problem_list = []

    olderYear_list, equalYear_list, newerYear_list, problemYear_list = categorizeSubdirs(oldest_dt.year, dir_path))

    for equalYear_path in equalYear_list:
        olderMonth_list, equalMonth_list, newerMonth_list, problemMonth_list = categorizeSubdirs(oldest_dt.month, equalYear_path))

        for equalMonth_path in equalMonth_list:
            olderDay_list, equalDay_list, newerDay_list, problemDay_list = categorizeSubdirs(oldest_dt.day, equalMonth_path))

    return remove_list, problem_list

The three nested loops at the end could be made less repetitive at the cost of code complexity. I don't think that it's worth it, though reasonable people can disagree. All else being equal, I prefer simpler code to slightly more clever code; as they say, reading code is harder than writing it, so if you write the most clever code you can, you're not going to be clever enough to read it. :/

share|improve this answer
add comment

Your Answer


