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 -- http://mail.python.org/mailman/listinfo/python-list