Merging two files into one .CSV

Question

I'm relatively new to Python scripting. I made a script that takes two raw data files and merges them into one CSV file, but it takes a long time to complete. Are there any logic problems in this code?

input_fileVms = open( 'vms.csv', 'rb')

if site == 'eg':
    output_merge = open('mergedFileEG.csv', 'wb')
elif site== 'fm':
    output_merge = open('mergedFileFM.csv', 'wb')

dataVms = csv.reader(input_fileVms)
writerMerge = csv.writer(output_merge,quoting=csv.QUOTE_ALL)

for lineVM in dataVms:
    input_fileUsers = open( 'users.csv', 'rb')
    dataUsers = csv.reader(input_fileUsers)
    new_line = strip(lineVM)
    OSFile = open('OS.csv', 'rb')
    OS = csv.reader(OSFile)
    i = 0
    for user in dataUsers:
        if i < 4:
            i = i + 1
        else:
            user = strip(user)
            if len(user) >= 4:

                idLine = str(user[1])

                if idLine in new_line:

                    for OS in OS:
                        system = OS[0]
                        if system in new_line:

                            if len(user) < 5:
                                writerMerge.writerow((str(new_line[2] + ',' + user[2] + "," + "Missing"+ ',' +OS[1])).split(','))
                            else:
                                writerMerge.writerow((str(new_line[2] + ',' + user[2] + "," +user[4]+ ',' +OS[1])).split(','))

            input_fileUsers.close()

Answer 1

File management

As already mentionned by others, opening files in your for loop can be the root of your slow performances. You can open all four of them beforehand since you won't be reading them in memory at once. If you need to read a single file (opened in, say, filehandler) several times, you can filehandler.seek(0) before reading it again.

But you also forget to close most of them. Especially OSFile in your code, which will keep as much file handlers opened in memory that there are lines in 'vms.csv'. And if your CSV grows big, you can run out of file descriptors at some point.

It is usualy suggested to combine open with with so that you don't need to remember to close files. But you already have a pretty much nested code that introducing a new level of nesting might not be that much of a good idea. Speaking of that…

Nesting

You have 8 levels of nesting. It impairs on readability and comprehension of your code. You should try to reduce that. One way of doing that is spliting your code into functions. Each function would be responsible of a for loop and thus will be limited to 2 or 3 levels of nesting. However, it will still be hard to get what the code does at a glance.

You can also make use of the continue keyword instead of using an else clause. This will allow you to remove nesting on ifs.

The last thing you can do, if possible, is to remove your ifs. For instance, if i < 4 is used to skip the first 3 lines of 'users.csv'. Better doing it explicitly by calling input_fileUsers.readline() 3 times. And add a comment to explain why ;)

On writing your output

The last thing to note is that you are building the line to write in the output file by:

1. concatenating bits of informations using + ',' + between them;
2. making a string out of the resulting string by calling str;
3. splitting that string on ','.

This is a waste of time. You'd be better of directly building a list out of your bits of informations.

This is also something you can take advantage of while reading files: since reading a line from a csv.reader returns a list, if you know that all columns have the same number of elements you can unpack them directly instead of having to access them by indices (see how I read from 'OS.csv' below).

Proposed improvements

# Backup to 'mergedFileFM.csv' anyway if site != 'eg'
output_filename = 'mergedFileEG.csv' if site == 'eg' else 'mergedFileFM.csv'
input_file_vms = open('vms.csv', 'rb')
input_file_users = open('users.csv', 'rb')
OSFile = open('OS.csv', 'rb')
output_merge = open(output_filename, 'wb')

vms = csv.reader(input_file_vms)
users = csv.reader(input_file_users)
os = csv.reader(OSFile)
output_writer = csv.writer(output_merge,quoting=csv.QUOTE_ALL)

for vm in vms:
    OSFile.seek(0)
    input_file_users.seek(0)
    # Skip 3 first lines of 'users.csv'
    input_file_users.readline()
    input_file_users.readline()
    input_file_users.readline()
    vm = strip(vm)
    for user in users:
        user = strip(user)
        if len(user) < 4:
            continue

        # No need to use str() here, lines read from files are already strings
        if user[1] not in vm:
            continue

        for system, os_name in os: # Assuming 'OS.csv' only contains 2 columns
            if system not in vm:
                continue

            try:
                user_value = user[4]
            except IndexError:
                user_value = "Missing"

            output_writer.writerow([vm[2], user[2], user_value, os_name])

input_file_vms.close()
input_file_users.close()
OSFile.close()
output_merge.close()

Answer 2

You should never do this:

for OS in OS:

Your loop variable "OS" is the same name as the iterator you're looping over. That is very likely to cause you problems. In some simple cases it will work, but makes for very unreadable code.

Secondly, for every line in the first file you are reading the ENTIRE second file! So you read the second file as many times as there are lines in the first. You need to separate the loops, read the first file (I like using input_fileVms.readlines() to put everything in a list of strings) and then read the second file. Once you have the contents of the two files in data structures, then you can merge them. Unless your files are in the 1GB range, this is what I would recommend. If you are just concatenating the files, you don't need csv.reader() because there is no reason to split out the strings and get the individual fields.

Answer 3

Puzzles

There are a number of things that puzzle me about the intention of this code. The most weird thing, I think, is if idLine in new_line and if system in new_line. That means that you are performing a join of the second column in users.csv to any column in vms.csv, and a join of the first column in OS.csv to any column in vms.csv. Typically, you would want to perform each join with one specific column in vms.csv. If that's not a bug, then there needs to be a comment explaining your intention.

I would love to find out what the columns in these files represent. The code could be rewritten more clearly if I knew more than the column numbers, but I can't help you with that given the current partial information in the question.

Naming

Notice the inconsistencies:

• File handles input_fileVms, output_merge, input_fileUsers, OSFile
• CSV reader/writers dataVms, writerMerge, dataUsers, OS

File handling

fileUsers is the only file handle that you properly close. Failing to OSFile is particularly bad, because you open it once per line in vms.csv.

As others have pointed out, opening and reading the entire users.csv and OS.csv for each line in vms.csv will lead to poor performance. You should read users.csv and OS.csv into memory just once.

The best way to take care of these problems is with a single with block:

if site in ['eg', 'fm']:
    with open('users.csv', 'rb') as user_input_file, \
         open('OS.csv', 'rb') as os_input_file, \
         open('vms.csv', 'rb') as vm_input_file, \
         open('mergedFile%s.csv' % site.upper(), 'wb') as merge_output_file:
        merge(merge_output_file, vm_input_file, os_input_file, user_input_file)

(As a general rule, you always want to call open() in the context of a with block.)

Miscellaneous

Take advantage of generators and list comprehensions to reduce excessive indentation.

In these lines…

if len(user) < 5:
    writerMerge.writerow((str(new_line[2] + ',' + user[2] + "," + "Missing"+ ',' +OS[1])).split(','))
else:
    writerMerge.writerow((str(new_line[2] + ',' + user[2] + "," +user[4]+ ',' +OS[1])).split(','))

… I don't understand why you need to call str(…), or why you concatenate and then split the results again. Also, I suggest changing the condition to len(user) <= 4 instead, so that you don't have both 4 and 5 as special numbers.

Suggested solution

To go with the with block above…

def merge(merge_output_file, vm_input_file, os_input_file, user_input_file):
    user_reader = csv.reader(user_input_file)
    user_list = [
        user for user in (
            strip(row) for i, row in enumerate(user_reader)
            if i >= 4                       # Skip first 4 lines
        ) if len(user) >= 4                 # Drop short lines
    ]

    os_reader = csv.reader(os_input_file)
    os_list = list(os_reader)

    vm_reader = csv.reader(vm_input_file)
    merge_writer = csv.writer(merge_output_file, quoting=csv.QUOTE_ALL)

    for vm_row in vm_reader:
        vm_row = strip(vm_row)
        for user in (user for user in user_list if str(user[1]) in vm_row):
            for os in (os for os in os_list if os[0] in vm_row):
                merge_writer.writerow([
                    vm_row[2],
                    user[2],
                    user[4] if len(user) > 4 else 'Missing',
                    os[1],
                ])