Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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:


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"


if __name__ == '__main__':

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

share|improve this question

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:

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

share|improve this answer

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

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 Py Jul 18 '11 at 21:31
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

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


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

Whenever you assemble text for future computer interpretation, whether by string concatenation or interpolation, alarm bells should go off: some escaping mechanism is essential! Failure to do so would expose you to a class of bugs akin to XSS and SQL injection.

According to the vCard version 2.1 specification, semicolons must be escaped with a backslash. The 3.0 specification mandates the use of escape sequences \\, \;, \,, and \n as well.

share|improve this answer

Your Answer


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.