9
\$\begingroup\$

A project to automate file organization in Linux using Python. I've always wanted to do this project way before I know how to code. Now that I did, I want to improve it even more.

Used Language: Python.

Compatible System: Linux.

Used Modules: OS Module.

How it works: When you save/move/copy a file to one of the main directories(ex: Downloads, Documents...etc), it will automatically move it to its specified directory. It will use both extension and the 2/3 letter code that I provide at the beginning of the file name. It will run in the background once I open my computer.

The program works only on files only, not directories (I don't usually have them and when I do, I want to do it personally).

Questions:

  1. How readable my code is?
  2. Are there any logical errors that I'm not aware of?
  3. How to improve it?
  4. Is there a better way to approach the problem?
  5. Overall, how would you rate it out of 10?

Thank you in advance.


CODE

FileOrganizer.py:

#!/usr/bin/env python

import os
import time
import os.path
from random import randint
from ExtraInfo import types, locations, docs, working_directories


class FileOrganizer:

    def __init__(self, directory_path):
        self.directory_path = directory_path

    def path_maker(self, root, file_name):
        """(str, str) -> str

        Returns a string containing the full path of a file,
        from root of the file and its name.

        >>> path_maker("/home/hama/Downloads", "area.cpp")
        "/home/hama/Downloads/area.cpp"
        >>> path_maker("/home/hama/Downloads/", "FuzzBuzz.py")
        "/home/hama/Downloads/FuzzBuzz.py"
        """

        return os.path.join(root, file_name)

    def extension_finder(self, path):
        """(str) -> str

        Takes in a string of full path of a file. If exists,
        returns a string of its extension, else returns False.

        >>> extension_finder("/home/hama/Downloads/area.cpp")
        ".cpp"
        >>> extension_finder("/home/hama/Downloads/FuzzBuzz.py")
        ".py"
        """

        if os.path.exists(path):
            if os.path.isfile(path):
                return os.path.splitext(path)[1]
        return False

    def category_selector(self, extension):
        """(str) -> str

        Takes in a string of an extension of a file. If not False,
        returns the category of the extension, else returns False.

        Precondition: The extension must be in one of the categories.

        >>> category_selector(".cpp")
        "programming-files"
        >>> category_selector(".mp4")
        "video"
        """

        if extension != False:
            for category in types:
                if extension in types[category]:
                    return category
                    break
            return False

    def get_prefix(self, path):
        """(str) -> str

        Takes in a string of full path of a file. If it is one of the doc
        categories returns the first 3 characters of name of the file, else 2.

        Precondition: A prefix of a specific directory should be provided
        at the beginning of the name of the file.

        >>> get_prefix("/home/hama/Downloads/umaMath-week11.pdf")
        "uma"
        >>> get_prefix("/home/hama/Downloads/pyFuzzBuzz.py")
        "py"
        """

        prefix = os.path.basename(path)
        if self.category_selector(self.extension_finder(path)) not in docs:
            return prefix[:2]
        else:
            return prefix[:3]

    def get_original_name(self, path):
        """(str) -> str

        Takes in a string of full path of a file. returns a string of
        the original file name without any prefix.

        Precondition: A prefix of a specific directory should be provided
        at the beginning of the name of the file.

        >>> get_original_name("/home/hama/Downloads/umaMath-week11.pdf")
        "Math-week11.pdf"
        >>> get_original_name("/home/hama/Downloads/pyFuzzBuzz.py")
        "FuzzBuzz.py"
        """

        file_name = os.path.basename(path)
        if self.category_selector(self.extension_finder(path)) not in docs:
            return file_name[2:]
        else:
            return file_name[3:]

    def random_name_generator(self, path):
        """(str) -> str

        Takes in a string of full path of a file. Generates a random
        integer at the end of the name of the file, the returns the new name.

        >>> random_name_generator("/home/hama/Downloads/umaMath-week11.pdf")
        "Math-week11.pdf"
        >>> random_name_generator("/home/hama/Downloads/pyFuzzBuzz.py")
        "FuzzBuzz.py"
        """

        file_name = os.path.splitext(path)[0]
        extension = os.path.splitext(path)[1]
        return f"""{file_name}-{randint(1, 250) % randint(1, 250)}{extension}"""

    def copy(self, file_source, destination_root):
        """(str, str) -> str

        Returns a string containing the full path of the newly moved file,
        from a full path of a file and root of the destination.

        Note: If a file with the same name already exists, a new name will be generated.

        >>> copy("/home/hama/Downloads/area.cpp", "/home/hama/Codes/C++/")
        "/home/hama/Codes/C++/area.cpp"
        >>> copy("/home/hama/Downloads/FuzzBuzz.py", "/home/hama/Codes/Python/")
        "/home/hama/Codes/Python/FuzzBuzz.py"
        """

        if not os.path.exists(self.path_maker(destination_root, self.get_original_name(file_source))):
            file_name = os.path.basename(file_source)
            file_destination = self.path_maker(
                destination_root, self.get_original_name(file_source))
            os.system(f"cp -pa {file_source} {file_destination}")
            return file_destination
        else:
            file_name = self.random_name_generator(self.path_maker(
                destination_root, self.get_original_name(file_source)))
            file_destination = self.path_maker(destination_root, file_name)
            os.system(f"cp -pa {file_source} {file_destination}")
            return file_destination


# Activated on these directories
paths = [FileOrganizer(f"{directory}") for directory in working_directories]
while True:
    for path in paths:
        # Get the files and directories in the root directory.
        for root, directories, files in os.walk(path.directory_path):
            root, directories, files = root, directories, files
            break

        # List the files in the directory
        list_of_files = []
        for file in files:
            list_of_files.append(path.path_maker(root, file))

        # Loop through the files and copy each one of them.
        proccess = True
        for file in list_of_files:
            if proccess:
                current_file = file

                file_category = path.category_selector(
                    path.extension_finder(current_file))
                if file_category in locations:
                    if locations[file_category].get(path.get_prefix(current_file)) != None:
                        destination_root = locations[file_category].get(
                            path.get_prefix(current_file))

                        # Check if there is a whitespace in the path, cause it cause infinite loop.
                        if not (" " in current_file):
                            new_file_destination = path.copy(
                                current_file, destination_root)
                        else:
                            continue
                        if os.path.exists(new_file_destination):
                            os.remove(current_file)

                        # Check if the file is moved and the proccess is done, otherwise wait until it is done.
                        if not os.path.exists(current_file) and os.path.exists(new_file_destination):
                            proccess = True
                        else:
                            proccess = False
                            while not proccess:
                                if not os.path.exists(current_file) and os.path.exists(new_file_destination):
                                    proccess = True
                                else:
                                    proccess = False
                                time.sleep(10)

        time.sleep(5)

# By: Hama
# Software Engineer to be.

ExtraInfo.py:

#!/usr/bin/env python
types = {

    'audio': ['.wpl', '.wma', '.wav', '.ogg', '.mpa', '.mp3', '.mid', '.midi', '.cda', '.aif'],

    'database': ['.csv', '.dat', '.db', '.dbf', 'log', '.mdb', '.sav', 'sqlite', '.sql', '.tar', '.xml'],

    'fonts': ['.fnt', '.fon', '.otf', '.ttf'],

    'image': ['.ai', '.bmp', '.gif', '.ico', '.jpeg', '.jpg', '.png', '.ps', '.psd', '.svg', '.tif', '.tiff'],

    'doc-presentation': ['.key', '.odp', '.pps', '.ppt', '.pptx'],

    'internet': ['.asp', '.srt', '.aspx', '.cer', '.cfm', '.cgi', '.htm', '.jsp', '.part', '.rss', '.xhtml', '.torrent'],

    'programming-files': ['.c', '.class', '.cpp', '.cs', '.h', '.java', '.pl', '.sh', '.swift', '.vb', '.php', '.html', '.css', '.js', '.py'],

    'doc-spreadsheet': ['.ods', '.xls', '.xlsm', '.xlsx'],

    'video': ['.3g2', '.3gp', '.avi', '.flv', '.h264', '.264', '.m4v', '.mkv', '.mov', '.mp4', '.mpg', '.mpeg', '.rm', '.swf', '.vob', '.wmv'],

    'doc-word': ['.doc', '.docx', '.odt', '.rtf', '.tex', '.wpd'],

    'doc-pdf': ['.pdf', '.epub', '.mobi'],

    'text': ['.txt']

}

locations = {

    'audio': {'na': '/home/hama/Music/Others'},

    'database': {'na': '/home/hama/Documents/Others/Database'},

    'fonts': {'na': '/home/hama/Documents/Others/Fonts'},

    'internet': {'na': "/home/hama/Documents/Others/Internet-Related"},

    'image': {'my': '/home/hama/Pictures/Myself', 'ot': '/home/hama/Pictures/Others', 'wa': '/home/hama/Pictures/Wallpapers'},

    'video': {'my': '/home/hama/Videos/Myself', 'ot': '/home/hama/Videos/Others', 'mv': '/home/hama/Videos/Movies', 'tu': '/home/hama/Videos/Tutorials', 'se': '/home/hama/Videos/Series'},

    'programming-files': {'ot': '/home/hama/Codes/Others', 'wb': '/home/hama/Codes/Web', 'cp': '/home/hama/Codes/C++', 'py': '/home/hama/Codes/Python'},

    'doc-spreadsheet': {'bop': "/home/hama/Documents/Books/Programming-Books", 'bon': "/home/hama/Documents/Books/Novels", 'boo': "/home/hama/Documents/Books/Others", 'duc': "/home/hama/Documents/Documents/Spreadsheet", 'tmp': "/home/hama/Documents/Temp", 'uot': "/home/hama/Documents/UKH/Undergraduate-I/Other-Documents", 'uma': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Engineering-Mathematics-II", 'udl': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Introduction-to-Digital-Logic-&-Electronics", 'usp': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Structured-Programming", 'uen': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/English-Composition-II"},

    'doc-presentation': {'bop': "/home/hama/Documents/Books/Programming-Books", 'bon': "/home/hama/Documents/Books/Novels", 'boo': "/home/hama/Documents/Books/Others", 'duc': "/home/hama/Documents/Documents/Presentations", 'tmp': "/home/hama/Documents/Temp", 'uot': "/home/hama/Documents/UKH/Undergraduate-I/Other-Documents", 'uma': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Engineering-Mathematics-II", 'udl': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Introduction-to-Digital-Logic-&-Electronics", 'usp': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Structured-Programming", 'uen': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/English-Composition-II"},

    'doc-word': {'bop': "/home/hama/Documents/Books/Programming-Books", 'bon': "/home/hama/Documents/Books/Novels", 'boo': "/home/hama/Documents/Books/Others", 'duc': "/home/hama/Documents/Documents/Word", 'tmp': "/home/hama/Documents/Temp", 'uot': "/home/hama/Documents/UKH/Undergraduate-I/Other-Documents", 'uma': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Engineering-Mathematics-II", 'udl': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Introduction-to-Digital-Logic-&-Electronics", 'usp': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Structured-Programming", 'uen': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/English-Composition-II"},

    'doc-pdf': {'bop': "/home/hama/Documents/Books/Programming-Books", 'bon': "/home/hama/Documents/Books/Novels", 'boo': "/home/hama/Documents/Books/Others", 'duc': "/home/hama/Documents/Documents/PDF", 'tmp': "/home/hama/Documents/Temp", 'uot': "/home/hama/Documents/UKH/Undergraduate-I/Other-Documents", 'uma': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Engineering-Mathematics-II", 'udl': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Introduction-to-Digital-Logic-&-Electronics", 'usp': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/Structured-Programming", 'uen': "/home/hama/Documents/UKH/Undergraduate-I/Semester-II/English-Composition-II"},

    'text': {'tx': "/home/hama/Documents/Documents/PDF"}
}

docs = ['doc-spreadsheet', 'doc-presentation', 'doc-word', 'doc-pdf']

working_directories = ["/home/hama/Downloads/", "/home/hama/Documents/", "/home/hama/Codes/",
                    "/home/hama/Desktop/", "/home/hama/Music/", "/home/hama/Pictures/", "/home/hama/Videos/"]

script.sh:

#!/bin/bash
nohup python3 -u /home/hama/Codes/Python/FileAutomationV1.0/FileOrganizer.py &
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Just a note: if you want this to run in the background and react to file activity on Linux, look into inotify. There's at least one Python wrapper for it. It'll be both faster than the polling you're doing now, and use fewer resources when nothing has changed. \$\endgroup\$
    – Useless
    Commented May 4, 2020 at 8:50
  • \$\begingroup\$ Thank you, of course, it is one of the things that I needed. \$\endgroup\$
    – Hama
    Commented May 5, 2020 at 6:26

3 Answers 3

23
\$\begingroup\$

Addressing your questions:

  1. Readability

    The docstrings are generally fine. Not too long, not too short.

    • Type hints:

      Your type hints should not go into the first line of the docstring. That line is reserved for a brief description of the function. Type hints go into the function signature directly, for example:

      def path_maker(self, root: str, file_name: str) -> str:
          """Brief description.
      
          More.
          """
          pass
      
    • PEP8:

      You followed conventions for the most part, which is good. Remaining points are:

      • module names are snake_case, that is ExtraInfo should be extra_info. Note how at the time of writing, SE's syntax highlighting for Python flags your ExtraInfo module as a class instead.
      • Globals are UPPERCASE, i.e. types, locations, docs, working_directories become TYPES, LOCATIONS, DOCS, WORKING_DIRECTORIES.
  2. Logical Errors

    • break is unreachable in category_selector:

      if extension != False:
          for category in types:
              if extension in types[category]:
                  return category
                  break
          return False
      

      and if extension != False can just be if extension.

3./4. How to improve it and a better approach

Enter pathlib. This module will be the single biggest improvement you can afford yourself in regards to your code. It will get rid of the filthy string manipulations of os.path. Paths will be treated as proper objects and the code will probably run OS-agnostically.

All of this assumes from pathlib import Path.

Without actually rewriting the entire thing for you, here are a couple of examples for the achievable simplifications:


os.path.join(root, file_name)

is turned into a method on a Path-like object:

root.join(file_name)

To borrow one of your doctests:

root = Path("home", "hama", "Downloads")
file_name = Path("area.cpp")
root.joinpath(file_name)

will output

PosixPath('home/hama/Downloads/area.cpp')

on Unix. Note the *args approach I took in declaring Path(). No slashes used. This gets rid of confusion about forward- and backward-slashes, so your script can run on any OS you want. You can also pass your normal strings, like home/hama/Downloads/area.cpp, or iterables of string like lists. pathlib understands a great deal of these.

file_name can even be whatever path you would want, not just a single name. This includes relative parts, aka ... Then can then be resolved using the resolve method.


In extension_finder,

if os.path.exists(path):
    if os.path.isfile(path):
        return os.path.splitext(path)[1]
    return False

can be simplified using

path.suffix

If path is a directory, the .suffix attribute will be an empty string. You could then just rely on the empty string behaving falsy in boolean contexts. The only thing you do with extension_finder is feeding it into category_selector. More on that method later.

Note that, given the method name, extension_finder, I feel like it is not that method's responsibility to check for file existence. You be the judge of that. For existence checking, Path objects have is_file(), which includes exists() functionality.

Also note how for that function, you pass self and path, but do not use self. This method is a prime candidate for a staticmethod.

@staticmethod
def extension_finder(path: Path) -> str:
    """Returns a path's file suffix.

    >>> extension_finder("/home/hama/Downloads/area.cpp")
    ".cpp"
    >>> extension_finder("/home/hama/Downloads/FuzzBuzz.py")
    ".py"
    """

    return path.suffix

Now that this method was simplified so strongly through the excellent pathlib capabilities, it stands to question whether you want to keep it around at all. In this form, I vote for no.


The logic in category_selector could then just be

for category in types:
    if extension in types[category]:
        return category

extension can now be an empty string, and as such is never found in types[category]. So that works as before. The return False is now omitted. If no match is found, the loop falls through and the function returns its default, None, which behaves like False in the boolean checks involving category_selection.

Note that if you want to retain the current behaviour, i.e. returning a special object, not an empty string from extension_finder if no extension was found, I suggest using None over False. Same goes for category_selector here. A return of None for a category makes much more sense: no category was found. Returning False behaves similarly, but is less clear.


get_prefix and get_original_name are duplicate code, they are each other's opposite. Really, only one method is required. This method could be called split_custom_prefix and return a tuple for you to unpack. It can look like

stem = path.stem
if self.category_selector(self.extension_finder(path)) not in docs:
    split = 2
else:
    split = 3

return stem[:split], stem[split:]

where stem is the filename without the last extension.

Note that 2 and 3 are magic numbers. You should find a way to avoid them, and codify their meaning into variables or logic with meaningful names. Towards this, among many other things, you could look to enum.


for root, directories, files in os.walk(path.directory_path):
    root, directories, files = root, directories, files
    break

is an interesting one. You do not use directories later on. Name such variables _, as per convention, to signal that you have to accept an argument in that position but do not actually use it.

root, directories, files = root, directories, files

Did you code this at 3 AM? ;) This line can just be deleted.

I am not aware of an os.walk equivalent in pathlib. If you really require the former, here is a better approach:

root, directories, files = next(os.walk(path.directory_path))

using next to trigger the generator object returned by os.walk once.


I am a fan of inverting logic like this

if not (" " in current_file):
    new_file_destination = path.copy(
        current_file, destination_root)
else:
    continue

to

if " " in current_file:
    continue
new_file_destination = path.copy(current_file, destination_root)

Gets rid of else (indentation) and is easier to understand.


Lastly, avoid globals. It is strange for the class to access globals. You can assign these as instance or class attributes, whatever fits better. For your categories, all instances can share the information, so class attribute it is. Look into the property decorator to play around with this. For example, this allows you to make the attributes immutable.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ Superb effort on explaining issues and promoting a different library for solution. Thank you for this detailed and comprehensive answer. \$\endgroup\$
    – Meto
    Commented May 3, 2020 at 12:04
  • 3
    \$\begingroup\$ I cannot thank you enough for the effort on explaining those. It was my first project, I just needed a good review like that, I will definitely go over them and improve them. Regarding pathlib, I was not aware of that module which seems to make my journey easier. Again thank you for your help and time. \$\endgroup\$
    – Hama
    Commented May 3, 2020 at 19:55
8
\$\begingroup\$

Aside from what others have already said, the main thing that jumps out at me is that you're polling for changes, which is wasteful.

On Linux, the proper solution is to use some bindings to the inotify API so you can receive IN_CLOSE_WRITE and IN_MOVED_TO events on the directories you're watching. That way, your monitor process can sleep indefinitely and the kernel will wake it when there's work to do.

\$\endgroup\$
4
  • \$\begingroup\$ Great information that I just needed. Yes, I was worked up about the program crashes due to its infinite loop and always running. Thank you! \$\endgroup\$
    – Hama
    Commented May 3, 2020 at 20:01
  • 1
    \$\begingroup\$ This answer could be improved with a link to the inotify module: pypi.org/project/inotify \$\endgroup\$ Commented May 4, 2020 at 5:32
  • \$\begingroup\$ Thank you, it will be helpful for sure. \$\endgroup\$
    – Hama
    Commented May 5, 2020 at 6:24
  • \$\begingroup\$ @DigitalTrauma I would have, but I remembered that, last time I checked, inotify bindings were in a bit of a transitional state and I didn't have time to track down which one was the current recommended one. \$\endgroup\$
    – ssokolow
    Commented May 5, 2020 at 21:11
7
\$\begingroup\$

I agree with all the points mentioned in the answer by @AlexPovel and I am not going to repeat them.

One thing that struck me as odd is that you have this class called FileOrganizer, but all it does is give you convenience functions for dealing with paths. What it explicitly does not do is organize your files! That is left as stand-alone code outside of the class.

I would make the moving code part of the class. This means that your global file type data structures should probably be class attributes. Afterwards you could do:

file_organizers = [FileOrganizer(path) for path in paths]
while True:
    for file_organizer in file_organizers:
        file_organizer.organize()
    time.sleep(5)

Currently you are also running a loop until the file copying has finished, checking every 10 seconds if the file is there yet. I would either go fully asynchronous (and keep track of files currently being copied) or use subprocess.run (Python 3.5+), which will only return once the command has completed.

The method could look something like this:

import subprocess

class FileOrganizer:

    ...

    def move(self, file, destination):
        ... # get the right names
        subprocess.run(["cp", "-pa", file, destination])
        if os.path.exists(destination):
            os.remove(file)
        else:
            print(f"Moving {file} failed.")
        return destination

    def organize(self):
        # Get the files and directories in the root directory.
        root, directories, files = next(os.walk(self.directory_path))
        root = Path(root)
        # List the files in the directory
        files = (root.joinpath(file) for file in files if " " not in file)

        # Loop through the files and copy each one of them.
        for file in files:
            file_category = self.category_selector(file.suffix)
            location = self.locations.get(file_category)
            if location is None:
                print("{file_category} is not a valid category")
                continue
            prefix = self.get_prefix(file)
            destination_root = location.get(prefix)
            if destination_root is None:
                print(f"{prefix} is not a valid prefix")
                continue
            self.move(file, destination_root)

Note that I added some debug output so it gets easier to see what the script is doing. You might want to move that to a log file using the logging module eventually.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for your suggestion. Yes, I had a big problem dealing with that, I looked for it but couldn't find a proper way that I would understand. Thus, I didn't look over that enough. Since I understand yours very well, I will fix that as well. Thank you again. \$\endgroup\$
    – Hama
    Commented May 3, 2020 at 19:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.