My first ever Python program, comments welcome

[Dave Angel]

On 07/21/2012 03:08 PM, Lipska the Kat wrote:
> Greetings Pythoners
>
> A short while back I posted a message that described a task I had set
> myself. I wanted to implement the following bash shell script in Python
>

You already have comments from Ian and MRAB, and I'll try to point out
only things that they did not.

Congratulations on getting your first program running.  And when reading
the following, remember that getting it right is more important than
getting it pretty.

> Here's the script
>
> sort -nr $1 | head -${2:-10}
>
> this script takes a filename and an optional number of lines to display
> and sorts the lines in numerical order, printing them to standard out.
> if no optional number of lines are input the script prints 10 lines
>
> Here's the file.
>
> 50    Parrots
> 12    Storage Jars
> 6    Lemon Currys
> 2    Pythons
> 14    Spam Fritters
> 23    Flying Circuses
> 1    Meaning Of Life
> 123    Holy Grails
> 76    Secret Policemans Balls
> 8    Something Completely Differents
> 12    Lives of Brian
> 49    Spatulas
>
>
> ... and here's my very first attempt at a Python program
> I'd be interested to know what you think, you can't hurt my feelings
> just be brutal (but fair). There is very little error checking as you
> can see and I'm sure you can crash the program easily.
> 'Better' implementations most welcome
>
> #! /usr/bin/env python3.2
>
> import fileinput
> from sys import argv
> from operator import itemgetter
>
> l=[]

I prefer to initialize an empty collection just before the loop that's
going to fill it.  Then if you later decide to generalize some other
part of the code, it's less likely to break.  So i'd move this line to
right-before the for loop.

> t = tuple

Even if you were going to use this initialization later, it doesn't do
what you think it does.  It doesn't create a tuple, it just makes
another reference to the class.  If you had wanted an empty tuple, you
should either do  t=tuple(), or better  t=()

> filename=argv[1]
> lineCount=10
>

I'd suggest getting into the habit of doing all your argv parsing in one
place.  So check for argv[2] here, rather than inside the loop below. 
Eventually you're going to have code complex enough to use an argument
parsing library.  And of course, something to tell your use what the
arguments are supposed to be.

> with fileinput.input(files=(filename)) as f:

fileinput is much more general than you want for processing a single
file.  That may be deliberate, if you're picturing somebody using
wildcards on their input.  But if so, you should probably use a
different name, something that indicates plural.

>     for line in f:
>         t=(line.split('\t'))
>         t[0]=int(t[0])
>         l.append(t)


>     l=sorted(l, key=itemgetter(0))
>

Your sample data has duplicate numbers.  So you really ought to decide
how you'd like such lines sorted in the output.  Your present code
simply preserves the present order of such lines.  But if you remove the
key parameter entirely, the default sort order will sort with t[0] as
primary key, and t[1] as tie-breaker.  That'd probably be what I'd do,
after trying to clarify with the client what the desired sort order was.

>     try:   
>         inCount = int(argv[2])
>         lineCount = inCount
>     except IndexError:
>         #just catch the error and continue       
>         None
>
>     for c in range(lineCount):
>         t=l[c]
>         print(t[0], t[1], sep='\t', end='')
>
> Thanks
>
> Lipska
>
>

A totally off-the-wall query.  Are you using a source control system,
such as git ?  It can make you much braver about refactoring a working
program.

-- 

DaveA