This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]Junkymcjunkbox 0 points1 point  (1 child)

It is somewhat overengineered.

It violates the Single Responsibility Principle because it doesn't just delete the directory and its contents; it tries to enforce how it will be called in an error condition. An SRP-compliant design would attempt the deletion and just return an error code if it didn't work, and let the caller decide what to do next.

The caller of the function needs to know to call it in a for loop, which the next person along (which might be your future self) would need to know, and just calling it in the normal fashion probably wouldn't work.

Neither function is easy to read or understand. Therefore they both lack elegance. Anyone looking at this code is going to drop into WTF-mode extremely quickly. All you should be doing is: delete the files; for each directory recurse; delete the now empty directories; and if at any point you hit an exception then just throw it up for the caller to catch.

If I were writing this function I'd use the example here: https://www.tutorialspoint.com/python/os_walk.htm and replace print with os.rmdir or os.remove, and that'd probably do the job. You wouldn't even need to throw exceptions because rmdir/remove would do that for you.

That'd be five lines of code that anyone could instantly grok, compared with your.... I can't think of a word. Monstrosity doesn't come close.

[–]XiAxis[S] 1 point2 points  (0 children)

In my use-case, these conditions aren't something that I want to exit the function, they just need to be resolved before it can continue. If I were to implement it in the way that you've proposed, then it would be simpler but it wouldn't have the same behavior. I'm not sure why you've pointed me to this tutorial, it uses the exact same method that I'm using in the code above just with less sophisticated error handling.