Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Context: I wrote a simple as an answer to a problem in Unix.SE and it was suggested to me to post it here for review.

A comma separated file is generated by an application. On the first column it contains a last name, on the second a first name and on the tenth column a telephone number. For each row of this csv an vcard entry should be generated.

This was my solution:

#!/usr/bin/python

import csv
import sys

def convert(file):
    reader = csv.reader(open(file, 'rb'))

    for row in reader:
        print 'BEGIN:VCARD'
        print 'VERSION:2.1'
        print 'N:' + row[0] + ';' + row[1]
        print 'FN:' + row[1] + ' ' + row[0]
        print 'TEL;HOME;VOICE:' + row[9]
        print 'END:VCARD'

def main(args):
    if len(args) != 2:
        print "Usage:"
        print args[0] + " filename"
        return

    convert(args[1])

if __name__ == '__main__':
    main(sys.argv)

Outputting the generated vcard on stdout was fine for me, I just redirected it into an appropriate file.

share|improve this question
add comment

4 Answers 4

Read about the with statement and ALWAYS use it with file operations

with open( somefilename, 'rb' ) as source:
    reader = csv.reader( source )
    for row in reader:
       etc.

That guarantees that the file is closed and all OS resources released.

share|improve this answer
add comment

Use sys.exit to indicate success or failure of the program.

file is a builtin python function, its best to avoid naming your variables the same thing.

for row in reader:
    print 'BEGIN:VCARD'
    print 'VERSION:2.1'
    print 'N:' + row[0] + ';' + row[1]
    print 'FN:' + row[1] + ' ' + row[0]
    print 'TEL;HOME;VOICE:' + row[9]
    print 'END:VCARD'

I'd recommend putting in something like

foo = row[0]

and then printing foo to make it clearer what the different values are.

share|improve this answer
add comment

Even for something this trivial, use argparse.

import argparse

parser = argparse.ArgumentParser(description='Convert a CSV or whatever.')
parser.add_argument('filenames',  type=argparse.FileType('rb'), nargs='+',
                   help='a file to convert')

args = parser.parse_args()
for sourcefile in args.filenames:
    convert( sourcefile )
share|improve this answer
    
(meta) Is it proper here to update the original question with the reviews applied? –  Vitor Jul 18 '11 at 21:31
1  
You could, but you'd have to leave the original code in place. However. I don't think that would accomplish much. You can beat a single example "to death" through over-analysis. What do you think you might learn from posting the revised code? –  S.Lott Jul 18 '11 at 22:11
add comment

Python Manual mentions that you should open a file with newline='' csv

Thus:

with open( somefilename, 'rb', newline='') as source:
   etc
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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