Improve my script to organize a video collection

Question

I've made this script the other day, and I need some input from you telling me how could I improve the code / style. There aren't problems with the code (it does the job) but I want it more organized and "simple" so that in 1 week I still remember what the hell was I thinking!

The problem was: I needed a program that moves for me some files, particularly, some video files, from one directory to another. All the video files are about Tv Shows, so they have this format:

Showname.SxxExx.Episode Title.mp4 (where S = season and E = episode)

But that's not it. I wanted also to create the apropriate folders for every Tv Show i moved in another directory. So if the directory "Show Name" doesn't exists, my script creates it and then checks if the season folder exists inside. If it doesn't, it creates that too and finally it moves the file.

Here is the code: http://bpaste.net/show/148514/

import string
import glob
import os
import shutil
import re
import sys
import hashlib
#############################################
from colorama import Fore, Back, init

init(autoreset=True)

#print(Fore.RED + 'some red text')
#print(Back.GREEN + 'and with a green background')
#print(Style.DIM + 'and in dim text')
#print(Fore.RESET + Back.RESET + Style.RESET_ALL)
#print('back to normal now')

#Fore: BLACK, RED, GREEN, YELLOW, BLUE, MAGENTA, CYAN, WHITE, RESET.
#Back: BLACK, RED, GREEN, YELLOW, BLUE, MAGENTA, CYAN, WHITE, RESET.
#Style: DIM, NORMAL, BRIGHT, RESET_ALL

# https://pypi.python.org/pypi/colorama#downloads

#############################################


class TvShow(object):
    def __init__(self, path):
        self.path = path  # The complete path to the video file. Example: D:\Movies\Somefolder\showname.showseason.avi
        self.show_name = None  # This will hold the show's name. Example: "xFactor" (shit program btw)
        self.show_season = None  # This will hold the show's season number. Example: "S06" for season 6.
        self.show_video_file_name = None  # The complete file name. Example: "Showname.SxxExx.Xvid.avi"

    def create(self):
        #First we split the path by "\" and put it in a list. REMEMBER: Linux uses slash and not backslash.
        splitted_path_by_slash = string.split(self.path, "\\")

        #Then we get the *last* item from the list, which will hold the complete name of our video file
        #and put it into the TvShow's variable.
        self.show_video_file_name = splitted_path_by_slash[-1]

        #Now we split the name by dots (.) to get the show's name and season
        splitted_video_file_name_by_dot = string.split(self.show_video_file_name, ".")

        #Here we finaly assign the show name to our variable.
        self.show_name = splitted_video_file_name_by_dot[0]

        #Here we get the "SxxExx" section and put it in a string
        tvshow_season_and_episode = splitted_video_file_name_by_dot[1]

        #Now we need to split that string to extract only the "Sxx" part.
        temp = string.split(tvshow_season_and_episode, "E")

        #Finaly we assign the value of season to it's variable.
        self.show_season = temp[0]


def checkIfValidFolder(destination, source):
    err = 0
    if os.path.exists(destination) is True and os.path.exists(source) is True:
        if os.listdir(source) == []:
            print("The source folder exists, but directory is empty. No videos?")
        else:
            for tvshowname in os.listdir(source):
                if re.match(r'(?P<name>[^.]+)\.S(?P<S>\d+)E(?P<E>\d+)\.(?P<title>[^.]+)\.(?P<ext>.*)', tvshowname) is None: #Regex explanation: http://regex101.com/r/gG9nK2
                    print("The following file is not valid: " + tvshowname)
                    err += 1

            if err == 0:
                print("Source and Destination folders exist! Nice one!")
                return True
            else:
                print("Errors were found")
                return False
                #do stuff...
    else:
        if os.path.exists(source) is False:
            print "The " + Fore.RED + "SOURCE" + Fore.RESET + " folder, " + str(source) + ", does not appear to exist."
        if os.path.exists(destination) is False:
            print "The " + Fore.RED + "DESTINATION" + Fore.RESET + " folder, " + str(
                destination) + ", does not appear to exist."



def md5_for_file(path, block_size=256*128, hr=True):
    '''
    Block size directly depends on the block size of your filesystem
    to avoid performances issues
    Here I have blocks of 4096 octets (Default NTFS)
    '''
    md5 = hashlib.md5()
    with open(path,'rb') as f:
        for chunk in iter(lambda: f.read(block_size), b''):
             md5.update(chunk)
    if hr:
        return md5.hexdigest()
    return md5.digest()


def summary(folders_created, subfolders_created):
    print("")
    print("------------------------------------------------------------------")
    if not folders_created:
        print ("No new show has been added to your collection master!")
        print("")
    else:
        print ("")
        print("New show(s) added:")
        for item in folders_created:
            print item

    print("")
    print("------------------------------------------------------------------")
    if not subfolders_created:
        print ("No new season has been added master!")
        print("")
    else:
        print ("")
        print("New season(s) added:")
        for item in subfolders_created:
            print item


def copyThisShit(from_dir, to_dir):
    i = 0
    j = 0
    new_fold = []
    new_sub_folder = []

    for filename in sorted(glob.glob(os.path.join(from_dir + "\\", '*.*'))):
        show = TvShow(filename)
        show.create()
        dest_show_folder = to_dir + "\\" + show.show_name
        if os.path.exists(dest_show_folder):
            dest_show_folder_season = dest_show_folder + "\\" + show.show_season
            if os.path.exists(dest_show_folder_season):
                if os.path.isfile(dest_show_folder_season + "\\" + show.show_video_file_name):
                    print(
                    "The file " + dest_show_folder + "\\" + show.show_season + "\\" + Back.GREEN + show.show_video_file_name + Back.RESET + " already exists. I'll SKIP it master")

                else:
                    print(
                    "The file " + dest_show_folder + "\\" + Back.RED + show.show_video_file_name + Back.RESET + " DOESN'T exist. I'll COPY it master")
                    #shutil.copy(filename, dest_show_folder_season)
                    copy_with_prog(filename, dest_show_folder_season + "\\" + show.show_video_file_name)
                    print("")
            else:
                print(
                "The subfolder " + dest_show_folder + "\\" + Back.RED + show.show_season + Back.RESET + " DOESN'T exist. I'll create it for you master")

                new_sub_folder.insert(i, dest_show_folder + "\\" + Fore.MAGENTA + show.show_season + Fore.RESET)
                i += 1

                os.mkdir(dest_show_folder_season)
                print ("The file " + dest_show_folder_season + "\\" + Back.RED + show.show_video_file_name + Back.RESET + " DOESN'T exist. I'll COPY it master")
                copy_with_prog(filename, dest_show_folder_season + "\\" + show.show_video_file_name)
                print("")
                #shutil.copy(filename, dest_show_folder_season)
        else:
            print (
            "The directory " + to_dir + "\\" + Back.RED + show.show_name + Back.RESET + " DOESN'T exists. I'll create it for you master")
            new_fold.insert(j, to_dir + "\\" + Fore.MAGENTA + show.show_name + Fore.RESET)
            j += 1
            i += 1
            new_sub_folder.insert(i, dest_show_folder + "\\" + Fore.MAGENTA + show.show_season + Fore.RESET)
            os.mkdir(dest_show_folder)
            print("The subfolder " + dest_show_folder + "\\" + Back.RED + show.show_season + Back.RESET + " DOESN'T exist. I'll create it for you master")
            os.mkdir(dest_show_folder + "\\" + show.show_season)
            dest_show_folder_season = dest_show_folder + "\\" + show.show_season
            print ("The file " + dest_show_folder_season + "\\" + Back.RED + show.show_video_file_name + Back.RESET + " DOESN'T exist. I'll COPY it master")
            copy_with_prog(filename, dest_show_folder_season + "\\" + show.show_video_file_name)
            print("")
            #shutil.copy(filename, dest_show_folder + "\\" + show.show_season)

    summary(new_fold, new_sub_folder)


def checkInput():
    valid = False
    while valid is False:
        source_folder = raw_input("Source Video Directory?")
        destination_folder = raw_input("Destination Video Directory?")
        if checkIfValidFolder(destination_folder, source_folder) is True:
            valid = True
            print ("Everything Looks good!")
            copyThisShit(source_folder, destination_folder)



class ProgressBar:
    def __init__(self, minValue = 0, maxValue = 10, totalWidth=12):
        self.progBar = "[]"   # This holds the progress bar string
        self.min = minValue
        self.max = maxValue
        self.span = maxValue - minValue
        self.width = totalWidth
        self.amount = 0       # When amount == max, we are 100% done
        self.updateAmount(0)  # Build progress bar string

    def updateAmount(self, newAmount = 0):
        if newAmount < self.min: newAmount = self.min
        if newAmount > self.max: newAmount = self.max
        self.amount = newAmount

        # Figure out the new percent done, round to an integer
        diffFromMin = float(self.amount - self.min)
        percentDone = (diffFromMin / float(self.span)) * 100.0
        percentDone = round(percentDone)
        percentDone = int(percentDone)

        # Figure out how many hash bars the percentage should be
        allFull = self.width - 2
        numHashes = (percentDone / 100.0) * allFull
        numHashes = int(round(numHashes))

        # build a progress bar with hashes and spaces
        self.progBar = "[" + '#'*numHashes + ' '*(allFull-numHashes) + "]"

        # figure out where to put the percentage, roughly centered
        percentPlace = (len(self.progBar) / 2) - len(str(percentDone))
        percentString = str(percentDone) + "%"

        # slice the percentage into the bar
        self.progBar = (self.progBar[0:percentPlace] + Back.BLUE + percentString
                        + Back.RESET + self.progBar[percentPlace+len(percentString):])

    def __str__(self):
        return str(self.progBar)

def copy_with_prog(src_file, dest_file, overwrite = False, block_size = 256*128):
    if not overwrite:
        if os.path.isfile(dest_file):
            raise IOError("File exists, not overwriting")

    # Open src and dest files, get src file size
    src = open(src_file, "rb")
    dest = open(dest_file, "wb")

    src_size = os.stat(src_file).st_size

    # Set progress bar
    prgb = ProgressBar(totalWidth = 79, maxValue = src_size)

    # Start copying file
    cur_block_pos = 0 # a running total of current position
    while True:
        cur_block = src.read(block_size)

        # Update progress bar
        prgb.updateAmount(cur_block_pos)
        cur_block_pos += block_size

        sys.stdout.write(
            '\r%s\r' % str(prgb)
        )

        # If it's the end of file
        if not cur_block:
            # ..write new line to prevent messing up terminal
            sys.stderr.write('\n')
            break
        else:
            # ..if not, write the block and continue
            dest.write(cur_block)
    #end while

    # Close files
    src.close()
    dest.close()

    #Check MD5 of the file to see if it was copied correctly.
    print(Back.YELLOW + Fore.BLACK + "Checking MD5 hash..." +  Fore.RESET + Back.RESET            ),
    if md5_for_file(src_file) == md5_for_file(dest_file):
        print (Back.GREEN + "MD5 GOOD!" + Back.RESET)
    else:
        print (Back.RED + "MD5 BAD!" + Back.RESET)
        raise IOError(
            "The two files don't have the same MD5 code!"
        )
     # Check output file is same size as input one!
    #dest_size = os.stat(dest_file).st_size
    #
    #if dest_size != src_size:
    #    raise IOError(
    #        "New file-size does not match original (src: %s, dest: %s)" % (
    #        src_size, dest_size)
    #    )


checkInput()

Answer 1

I notice that you have a lot of long strings sprinkled in your code.

" DOESN'T exists. I'll create it for you master"

Put those sorts of strings into constants. I did a quick count, and you have only 3 unique strings, yet your instance count of them is 7. Oh, and one of them had a typo, which would have made it 4 unique strings, if I hadn't fixed it. How you get rid of the duplicates is up to you, however I would do it like so (as per the PEP8 standard):

N_EX_COPY_CONST = " DOESN'T exist. I'll COPY it master"
EX_SKIP_CONST = " already exists. I'll SKIP it master"
N_EX_CREATE_CONST = " DOESN'T exist. I'll create it for you master"

As the poster above mentioned. You have naming issues, definitely. But they're compounded by your lack of proper use of functions/methods. Your "copyThisShit" function is about 50 lines long. One of the main reasons why it's so long is because it does too much. Issues with that function:

• You perform a large block of code on a single item. Yet you include the logic that loops through the items inside the same method. Rather, take the large block of code out as a separate method. Call it "copyThisShittyItem" if you like, and call it inside the loop.
• You seem to have repeated logic nested within itself. Is there any particular reason why you're hard-coding the structure of the data you're copying from/to? You check for a root folder, then you check for the season folder within it. Perhaps a more generic solution might work better? I can't really offer more info than that, without knowing your use case.
• You include an index (i and j) while going through a list. Since the order is going in one direction, and doesn't get affected by anything, you might as well just append to the list, which would save you keeping track of an index and incrementing it.
• Moving the main body of the copyThisShit function into a separate function will require you to pass back the "summary". From the looks of it, summary is a two lists of string. Either return them as a tuple, or create a simple data structure to pass them back, like your "TvShow" class.
• As per above, it becomes very important to know how to pass data around. Keeping it all in semi-global scope does make things easier, but you shoot yourself in the foot by leaving you very incapable of separating code out into smaller chunks that do one thing each.

On a side note, I can recommend a really good book by Martin Fowler, called refactoring. It codifies a lot of the common concepts that go into refactoring code, and it helped me quite a bit to get rid of some of my rather bad habits when programming. Here is a link to the Amazon page for it.

Answer 2

I'm just going to mention a few quick observations rather than do a thorough review.

You're concatenating backslashes everywhere. Use os.path.join() like you're supposed to. (The one place where you call os.path.join(), you're manually adding a backslash unnecessarily.) Conversely, in TvShow.create(), call os.path.split() instead of string.split(…, "\\").

Why does TvShow have an __init__() and a separate create() method? That suggests that your __init__() is a half-assed constructor, producing objects in a not-quite-usable state. If it's possible for objects to be in a weird state, then your class is poorly designed.

Your checkIfValidFolder() does not consistently return a value. Make up your mind what it is supposed to do.

In copyThisShit(), the logic is repetitive. The following flow would work better:

for filename in …:
    if not os.path.exists(to_dir):
        # Dest folder missing? Fix it!
        …
    if not os.path.exists(dest_show_folder):
        # Dest show folder missing? Fix it!
        …
    if not os.path.exists(dest_show_folder_season):
        # File not there? Fix it!
        …

Instead of concatenating blahblah + RED + string + RESET + blah all the time, find a way to call some function to set and reset the color like blahblah + fgRed(string) + blah. It would be cleaner and less error prone.

You have some naming issues. md5_for_file() is your only function_with_underscores. checkInput() is a crazy name for your main function. Who would guess that check…() does more than just checking something? Also, "prog" is usually short for "program", I would have guessed that copy_with_prog() meant "copy using a program" (like copy.exe or rsync), not "copy with a progress bar".