Ee62f45441fbf8409dfd4adaa40a74aa

From the useless speculations dept... how would you rewrite this? Those ifs at the beginning are repetitive, the main loop inside the try block feels like a no-no according to PEP-8 and the finally block could try to close a file that was never open in the first place. What do you think?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
try:
    if options.input_file:
        infile = open(options.input_file, 'r')
    else:
        infile = sys.stdin

    if options.output_file:
        outfile = open(options.output_file, 'w')
    else:
        outfile = sys.stdout

    for line in infile:
        # ...

except IOError, e:
    print "Can't open file %s." % e.filename
    sys.exit(e.errno)
finally:
    if options.input_file: infile.close()
    if options.output_file: outfile.close()

Refactorings

No refactoring yet !

A33619d2388462268ea2d2517762cb81

Dave Cridland

August 28, 2008, August 28, 2008 07:56, permalink

1 rating. Login to rate!

Well, the finally block isn't important - Python closes files when the object lifetime ends - basically doing what you want. Besides which, closing stdin or stdout isn't a disaster anyway, it's pretty normal behaviour.

The except block is reporting the wrong error - you might have got IOError during reading or writing, after all - in general, whilst I like having a catch-all exception handler, Python has it anyway, so catch specific errors where they occur.

sys.exit should indicate what the error is, not what the error code was. In you case, it'll return, for example, EPERM, without indication of whether it's the input or output file at fault.

I prefer wrapping main code in a traditional if __name__ == '__main__', at least if the code is at all likely to end up being imported.

And by the way, PEP-8 applies to code within the standard library, rather than standalone scripts. A lot of it is helpful, of course, even here.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
if __name__ == '__main__':
    try:
        if options.input_file:
            infile = open(options.input_file, 'r')
        else:
            infile = sys.stdin
    except IOError, e:
        print >>sys.stderr, "Cannot open input file %s." % (options.input_file)
        sys.exit(1)

    try:
        if options.output_file:
            outfile = open(options.output_file, 'w')
        else:
            outfile = sys.stdout
    except IOError, e:
        print >>sys.stderr, "Cannot open output file %s." % (options.output_file)
        sys.exit(2)

    for line in infile:
        # ...
Avatar

Malte

August 28, 2008, August 28, 2008 22:17, permalink

No rating. Login to rate!

I agree with most changes here, but I don't think the exit code should differ depending on what kind of error you had. It's customary to have 2 for bad usage (i.e. errors parsing the command-line arguments) and 1 for essentially all other errors, so sys.exit actually has the "1 for error" built-in: If you call it with a printable argument, it will print this to sys.stderr and use an exit code of 1. This is exactly what you want.

Style issues: The use of "" vs. '' appears inconsistent. I'd stick to one one style unless you really want the other style to avoid escaping something. Also, I'd omit the default "r" for opening a file for reading, and avoid introducing a variable (e) that isn't used.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
if __name__ == "__main__":
    try:
        if options.input_file:
            infile = open(options.input_file)
        else:
            infile = sys.stdin
    except IOError:
        sys.exit("Cannot open input file %s." % options.input_file)

    try:
        if options.output_file:
            outfile = open(options.output_file, 'w')
       else:
            outfile = sys.stdout
    except IOError:
            sys.exit("Cannot open output file %s." % options.output_file)

    for line in infile:
        # ...
Ee62f45441fbf8409dfd4adaa40a74aa

agnul

August 29, 2008, August 29, 2008 07:55, permalink

No rating. Login to rate!

Me too! All those changes make perfect sense, the only "problem" I see is that having a try block for each open goes straight back to the old days and C, only more verbose. Anyone fancy a flame war on Exceptions vs. Error codes? :-)

Thanks both of you.

F17c0b98ba353560769ef225c5c5c9fe

luntain

September 1, 2008, September 01, 2008 13:09, permalink

No rating. Login to rate!

I have extracted the commonality between handling of the input and output into a helper function. I called it safe_open for lack of better idea.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
if __name__ == "__main__":
    def safe_open(maybe_filename, default, options='r'):
        if maybe_filename:
            try:
                return open(maybe_filename, options)
            except IOError:
                sys.exit("Cannot open input file %s." % maybe_filename)
        return default

    infile = safe_open(options.input_file, sys.stdin)
    outfile = safe_open(options.output_file, sys.stdout, 'w')

    for line in infile:
        # ...
Avatar

Patrick

November 14, 2008, November 14, 2008 20:28, permalink

No rating. Login to rate!

It would probably be a better idea to group all the code which could result in Very Bad Things™ in one place. So instead of merely opening the file safely and subsequently having to make sure you don't choke while reading/writing or closing the file, just read/write the whole thing safely. This keeps you from doing try:...except every 3 lines when you want to manipulate the file data. It also puts the cost of I/O in one place.

More (maybe even too) verbose? Probably for a short script like this. But if you ever plan on reusing it, I think you'll find it usually pays off later. I didn't test this, but I think it reproduces what you were going for faithfully.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
if __name__ == '__main__':
    def safe_read(filename, options='r'):
        contents = []
        try:
            fd = open(filename, options)
            tmp = fd.readlines()
            fd.close()
            contents = tmp
        except IOError:
            print "Can't open file %s." % maybe_filename
        return contents

    def safe_write(filename, contents, options='w'):
        success = False
        try:
            fd = open(filename, options)
            fd.write(contents)
            fd.close()
            success = True
        except IOError:
            print "Can't open file %s." % filename
        return success

    # Try for the file read; if that fails fall back on stdin
    for line in safe_read(options.input_file) or sys.stdin:
        # ...

    # If you want to write something...
    some_output = '...'
    safe_write(options.output_file or sys.stdout, some_output)
Avatar

Patrick

November 14, 2008, November 14, 2008 20:34, permalink

No rating. Login to rate!

I had it slightly wrong above. This is starting to feel like a bit of pythonic abuse, but the meat of the code is still succinct and fairly functional.

Change:

1
2
3
4
5
safe_write(options.output_file or sys.stdout, some_output)

# =>

safe_write(options.output_file, some_output) or safe_write(sys.stdout, some_output)
Avatar

Patrick

November 14, 2008, November 14, 2008 20:38, permalink

No rating. Login to rate!

Yet more wrong... sys.stdout is a file descriptor, not a filename, and can't be opened. So you'd have to just do a straight write. I suppose it's relatively safe to assume that if stdout writes are failing it's the least of your program's worries.

I'm done. D:

5e7e68e1cdaea224d93da209afb28e9d

John

November 19, 2008, November 19, 2008 05:55, permalink

No rating. Login to rate!

Am I missing something or would this do what you want?

1
2
3
4
import fileinput

for line in fileinput.input([options.outputfile]):
    # Do stuff with line here

Your refactoring





Format Copy from initial code

or Cancel