3
\$\begingroup\$

I'm a beginner and i just started with automate the boring stuff with python course and reached his first os lesson, so i decided to make a recursive directory size finder since apparently os.path.getsize doesn't get the size of folders. Need a way to improve my code or advice on coding tricks, to make it more efficient

import os
class FileSizeFinder:

    def calculate_directory_size_in_bytes(self, directory: str, totalsize=0):
        for filename in tuple(os.listdir(directory)):
            current_directory = os.path.join(directory, filename)
            if os.path.isdir(current_directory):
                totalsize += FileSizeFinder.calculate_directory_size_in_bytes("self", current_directory)
            if not os.path.isfile(current_directory):
                continue
            totalsize += os.path.getsize(current_directory)
        return totalsize

    def get_file_size(self, directory: str, totalsize=0):
        total_size_in_bytes = FileSizeFinder.calculate_directory_size_in_bytes("self", directory)
        if total_size_in_bytes > 1024000000:
            totalsize = "Size in GB: " + str(round(total_size_in_bytes / 1024000000, 3))
        elif total_size_in_bytes > 1024000:
            totalsize = "Size in MB: " + str(round(total_size_in_bytes/ 1024000, 3))
        elif total_size_in_bytes > 1024:
            totalsize = "Size in KB: " + str(round(total_size_in_bytes / 1024, 3))
        return totalsize

print(FileSizeFinder.get_file_size("self", r"E:\Utilities_and_Apps\Python"))
\$\endgroup\$
5
\$\begingroup\$
  1. If there's no reason to make it a class, don't make it a class. (A good clue is that your class doesn't have any instance variables, and that both of its methods could be static without changing the way they work.)

  2. Both of your functions return the same information (formatted differently), but one calls it file_size and the other calls it directory_size even though it's the same number. In addition, neither has a type annotation for the return value. This all adds up to make it hard to figure out what each function does. Maybe it'd be better to just have one function that gets the size and a seperate one that formats it as a string?

  3. The totalsize parameter does not seem to serve any purpose as a parameter, and should be removed.

  4. There's no reason to convert os.listdir into a tuple; you can iterate over it as-is.

  5. current_directory is not necessarily a directory, so this name is confusing.

  6. Your size formatting logic has an obvious gap in its if...elif chain. Adding an else case fixes that.

  7. This is a style thing, but I think f-strings are nicer looking than explicitly converting to str and using concatenation.

  8. Your GB/MB/KB definitions are both copy+pasted and wrong. Maybe this whole if...elif construction could be done more cleanly by turning the unit definitions into a table (where each unit size is defined exactly once) and iterating over the table?

  9. Your recursive function would actually be simpler if it accepted any arbitrary path. That way you wouldn't have to do as much logic inside the body of your iteration over the os.listdir results, and could actually just do it as a simple sum of recursive calls.

  10. It's easier to test your code if you write a __main__ block that accepts a command line parameter!

Here's my massaged version of your code (does basically the same thing, but it uses the units as I've defined them rather than your hybrid 1024 * 10^N units -- having them defined the way I did it here makes it very easy to change if you'd rather make them 2^10, 2^20, etc).

import os
import sys

def recursive_get_size(path: str) -> int:
    """Gets size in bytes of the given path, recursing into directories."""
    if os.path.isfile(path):
        return os.path.getsize(path)
    if not os.path.isdir(path):
        return 0
    return sum(
        recursive_get_size(os.path.join(path, name))
        for name in os.listdir(path)
    )

def format_size(num_bytes: int) -> str:
    """Formats a size (given in bytes) into a human-readable string."""
    for unit_name, unit_size in [
        ("GB", 10**9),
        ("MB", 10**6),
        ("KB", 10**3),
    ]:
        if num_bytes > unit_size:
            return f"Size in {unit_name}: {round(num_bytes / unit_size, 3)}"
    else:
        return f"Size in bytes: {num_bytes}"

if __name__ == '__main__':
    print(format_size(recursive_get_size(
        sys.argv[1] if len(sys.argv) > 1 else r"E:\Utilities_and_Apps\Python"
    )))
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ yeah i just had this thought and tried to implement it with just the knowledge i've acquired, since you earn experience by doing mistakes and fixing them. \$\endgroup\$ – MahdeenSky Apr 9 at 13:53
  • 1
    \$\begingroup\$ yup, and getting code reviews is exactly the right thing to do to get ideas about better ways to do things! :) \$\endgroup\$ – Samwise Apr 9 at 13:55
4
\$\begingroup\$

Just a few remarks from me:

  1. Rather than hardcoding the path in your code it would be better to use a command line argument, then it can be used like an OS command such as dir or du. That is more user-friendly since your aim is automation.
  2. There is no exception handling in your code, so it will crash on permission error, which could easily happen, then you have to decide whether to skip or stop execution - but if you don't handle errors the figures are going to be incomplete, misleading and useless
  3. Overall the function can be implemented more efficiently using os.scandir (here's why) instead of listdir (your choice)
  4. Let's mention os.lstat too, which does not follow symbolic links because you probably don't want to count them - I notice that you are on Windows though and probably didn't give thought to the portability of this code
  5. You could also have taken advantage of os.walk.
  6. In fact, if you are only working on Windows and never develop on Linux machines, then you could have used Powershell and a one-liner could suffice. I would not have bothered with Python personally, not for this job.

In a way this is reinventing the wheel (I just saw the tags now lol but it wasn't me) but there is education value nonetheless. However I would have made more research to find out how programmers have tackled this task before. And then, I would try to improve their code if possible or customize it. The point is to learn from others and not just guess everything and do it all from scratch.

Unsurprisingly what you are trying to do has already been done countless times. I was going to quote the same SO post as Reinderien. So make sure you read all of it. Because even the accepted answer can be improved (as is often the case on SO). What is evident is that the task can be accomplished in a more straightforward way.

What I would have done otherwise is read the manual page about os functions, even if it means skimming through, to have an overview of the functions available, then decide on the most adequate functions for the job. As you can see, the Python language is rich enough to provide different ways of doing things.

Python can certainly help you automate the boring stuff, it's just that in this particular case the benefit is not that obvious, considering that your script does not have any options and therefore does less than the tools that already exist.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

File sizes

1,048,576 bytes are one MiB. 1,000,000 bytes are one MB. 1,024,000 bytes are not anything at all. You should choose the first or second one; the first one goes up in factors of 1,024.

Bug

What happens when you pass 315 to get_file_size? totalsize will not be set.

Iteration and recursion

Have a read through this:

https://stackoverflow.com/a/1392549/313768

which has some great suggestions. Your recursive solution is not the end of the world, but the class implementation is problematic. You've made a class but have not instantiated it, and are passing the string 'self' into the self parameter, which is not how this should work. At the absolute least, you should be instantiating FileSizeFinder and then calling the two methods on that instance. Better: define the methods to be @staticmethod so that self is not necessary. Much better: do not use a class at all, and simply have your two functions.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ i've been looking for this thing called @staticmethod for so long, wish i knew it, yep i didn't go into dept with object oriented programming so i have no idea what self is for.. \$\endgroup\$ – MahdeenSky Apr 9 at 13:54

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.