Parsing video files from a given directory

Question

I have written a little class that is basicly looking for video files in given directory and then parsing some info about those files. Finally it saves everything in a list.

To properly run this you need ffmpeg.exe (to get file duration).

Can you please give me some hints how can I improve my code? I am not sure if I have correctly used try-except instructions.

How would you advise me to store the data? Now i saved it as CSV but I think that is a bad idea - reading will be difficult.

This code works, but now I would like it to be well-written and clear as crystal.

import os
from stat import * # ST_SIZE etc
import subprocess
import re
import csv

class  FileList:
    def __init__(self, directoryPath):
        self.directoryPath = directoryPath #FileList object will hold directory path that is being searched
        self.filelist=[] #this will hold list of all found files
        self.skippedFiles=[] #this will hold files that were skipped while searching

    def filelength(self, filePath):
        process = subprocess.Popen(['ffmpeg',  '-i', filePath], stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
        stdout, stderr = process.communicate()
        matches = re.search(r"Duration:\s{1}(?P<hours>\d+?):(?P<minutes>\d+?):(?P<seconds>\d+\.\d+?),", stdout, re.DOTALL).groupdict()

        #print process.pid
        process.kill()
        duration = {'hours': matches['hours'], 'minutes' : matches['minutes'], 'seconds' : matches['seconds'], 'total_in_sec' : float(matches['seconds']) + 60 * float(matches['minutes']) + 3600 * float(matches['hours'])}
        return duration

    def scanDirectory(self, allowedFormats):

        #convert allowedFormats to lower case (self.fileFormat will also be converted)
        #we need to do so because "mp4" != "MP4"
        [x.lower() for x in allowedFormats] 


        for path, dirs, files in os.walk(self.directoryPath):
            for f in files:
                #Try reading stats of the file (Absoulte path) if it doesn't fail then we can continue parsing
                try:
                    self.absolutePath = path + '\\' + f
                    self.st=os.stat(self.absolutePath)
                    self.fileName_split = f.split(".")
                    self.fileFormat = self.fileName_split[-1]

                except IOError:
                    print "Failed to get information", f
                except:
                    print "Error reading file info or getting file format"
                else:
                    #There was no error reading filename, so we can continue parsing files
                    if self.fileFormat.lower() in allowedFormats:
                        try:
                            self.duration = self.filelength(self.absolutePath)
                        except Exception as inst:
                            print type(inst)     # the exception instance
                            print inst           # __str__ allows args to be printed directly
                            print "You are probably trying to get filelength of a file that isn't a video file. Hence ffmpeg fails."
                        else:
                            #There was no error reading movie file so we can update metadata
                            duration_string = "[%s:%s:%s]" % (self.duration['hours'],self.duration['minutes'],self.duration['seconds'])

                            self.metadata = {'fullpath' : self.absolutePath ,
                                        'filename' : f ,
                                        'format' : self.fileFormat, 
                                        'filelength' :  duration_string,
                                        'filesize' : self.st[ST_SIZE],
                                        'ratio' : (self.duration['total_in_sec'] / self.st[ST_SIZE]) * 1000*1000}
                                        #time/size ratio, *1000 to convery bytes to kilobytes and then *1000 to scale the ratio to whole numbers
                                        # 0-4 bad, 4-6 not good, 6-8 good, 8> great
                            self.filelist.append(self.metadata)
                            print self.metadata
                    else:
                        self.metadata_skipped = {'fullpath' : self.absolutePath, 'format' : self.fileFormat} 
                        self.skippedFiles.append(self.metadata_skipped)

    def dataOutput(self, filelist):
        self.writer = csv.writer(open('dict.csv', 'wb'))
        self.writer.writerow(["number of files", filelist.__len__()])

        for i in filelist: #filelist contains dictionaries with metadata about movie file
            for key, value in i.items(): #take each pair: [key,value] in dictionary i.items returns all the [key,value] pairs
                self.writer.writerow([key, value]) #write each [key,value] pair as a single line, separate them with comma

            self.writer.writerow("")#write blank line to separate each dictionary (so each file metadata)


if __name__ == "__main__":
    my_FileList = FileList("C:\Movies")
    my_FileList.scanDirectory(["mp4", "mkv", "flv", "wmv", "avi", "mpg", "mpeg"])
    my_FileList.dataOutput(my_FileList.filelist)

Answer 1

Overall your code looks pretty good. It would be nice if you broke your lines, though; they're very long right now, so I have to scroll horizontally to read them. You also mix naming conventions in a somewhat weird way with fileName_split. It would be better to stick with one convention; you've chosen camel case, so fileNameSplit or splitFileName.

scanDirectory is quite long, and the nesting gets pretty deep, so it might be good to try and break that out into separate functions.

I'm not sure why metadata, duration, absolutePath, or fileFormat have to be instance variables (which they are, if you write self.metadata). It looks like you're defining and using them within just one method, so they can be local variables.

---

I recommend against having print statements inside classes. One of the benefits of classes is code reuse. Having print statements inside class methods makes the class less reusable. Some of these can just go away, or they can become logging statements if you want. The ones inside your except clauses should probably become rethrows. They can also be logging statements if you want. I would probably write it like this:

try:
    self.absolutePath = path + '\\' + f
    self.st=os.stat(self.absolutePath)
    self.fileName_split = f.split(".")
    self.fileFormat = self.fileName_split[-1]
except IOError:
    raise IOError("Failed to get information {}".format(f))

In your main program, you can catch the rethrown exception and print its message. This is better than a version without a try/except clause because we can add information to the exception about what went wrong to make it easier to debug. This is better than the version with a print statement because callers of the method can decide how they want to deal with this, instead of being forced to have a print.

It's also usually considered bad style to use a bald except clause. For debugging, just let the exception stop your program. Printing "Error reading file info or getting file format" is not very informative, and doing so can also mask the real cause of bugs. Look at the following highly exaggerated code:

def factorial(n):
    return n * factorial(n-1)

try:
    f = open("factorials.txt")
    f.write(factorial(100000000000))
except:
    print("File opening failed.")

If you run that code, it will print "File opening failed". Of course, that's not really what happened; what happened is that factorial(1000000000000) exceeded the maximum recursion limit and the code threw a RuntimeException. But the except clause is misleading us. Now imagine that this code is much, much larger, and also you didn't write it. It could take days or weeks (depending on complicated the code is) to figure out that the code is lying to you.

---

Your code is written in a very Java-like style: your methods don't return values, but operate by mutating class instance variables. There's nothing wrong with this per se, but it does have some negative effects:

• It makes the code longer because you have to keep writing self.filelist.append(this), self.skippedfiles.append(that).
• It makes the code harder to test. You have to establish a bunch of context before you can get a result from a method like scanDirectory or dataOutput.

At the very least, I would have my methods return self. Then you can chain calls like in Ruby or JQuery; your main can be:

if __name__ == "__main__":
    myFileList = FileList("C:\Movies").scanDirectory(["mp4", "mkv", "flv", "wmv", "avi", "mpg", "mpeg"])
    myFileList.dataOutput(my_FileList.filelist)

But consider cutting down on the amount of context your class stores, passing more things in as arguments, and returning results, e.g. make scanDirectory take a directory and a list of allowed formats as arguments, and return a tuple containing the list of found movies and the list of skipped movies. Then you could write something like:

found, skipped = moviesList.scanDirectory("C:\Movies", 
                                          ["mp4", "mkv", "flv", 
                                           "wmv", "avi", "mpg", "mpeg"])
moviesList.dataOutput(found)
print("Skipped ")
for skipped_movie in skipped: 
    print(skipped_movie)

This makes testing easier; I can pass in whatever I want without having to build any context, and I can check the return value. Using return values also lets you use doctests, which are one of the cool features of Python that no other language I know of has.

---

Finally, you asked about the data format. Which data format works best really depends on how much data you have and what you plan to do with the data.

For instance, I wrote a script a while ago that worked with a CSV file with five million entries, extracting around 48,000 of them. It took about two and half minutes to finish, but for the kind of batch use it was intended for, that was fine. I could just collect the results into a new file and use them.

If you intend to do frequent lookups, it's hard to beat a relational database for speed. Python has built-in support for SQLite3, a lightweight relational database. A simpler option that avoids extra software is Python's shelve module; see http://pymotw.com/2/shelve/. If you intend to pass the data round over a network, JSON is a good choice, and you can store the JSON in SQLite or Postgres or MongoDB or numerous other places. There might be cases where XML is the best choice. It really all depends on what you want out of your data store. There are lots of cases where CSV files are perfectly fine; they have the advantage of being human-readable and very easy to parse, for instance.