Re: My first ever Python program, comments welcome

Discussion in 'Python' started by Ian Foote, Jul 21, 2012.

  1. Ian Foote

    Ian Foote Guest

    On 21/07/12 20:08, 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
    >
    > 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=[]
    > t = tuple

    What is this line supposed to do? If you're trying to make an empty
    tuple, you can write:
    t = ()
    But I don't think this is needed at all.
    > filename=argv[1]
    > lineCount=10
    >
    > with fileinput.input(files=(filename)) as f:
    > for line in f:
    > t=(line.split('\t'))
    > t[0]=int(t[0])
    > l.append(t)
    > l=sorted(l, key=itemgetter(0))
    >
    > try:
    > inCount = int(argv[2])
    > lineCount = inCount

    I don't think you need to split this into two lines here.
    try:
    lineCount = int(argv[2])
    should work.
    > except IndexError:
    > #just catch the error and continue
    > None

    I would use pass instead of None here - I want to "do nothing" rather
    than create a None object.
    > for c in range(lineCount):
    > t=l[c]
    > print(t[0], t[1], sep='\t', end='')
    >
    > Thanks
    >
    > Lipska
    >


    My only other point is that you might find it helpful to use slightly
    more verbose names than l or t - its not immediately obvious to the
    reader what these are intended to represent.

    Regards,
    Ian
    Ian Foote, Jul 21, 2012
    #1
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Paul Scott
    Replies:
    4
    Views:
    236
    Paul Scott
    Apr 8, 2008
  2. MRAB
    Replies:
    4
    Views:
    178
    Dave Angel
    Jul 22, 2012
  3. Dave Angel
    Replies:
    8
    Views:
    244
  4. Peter Otten
    Replies:
    0
    Views:
    152
    Peter Otten
    Jul 22, 2012
  5. Ivan@work
    Replies:
    0
    Views:
    147
    Ivan@work
    Jul 23, 2012
Loading...

Share This Page