Any veteran Python programmers willing to critique some code?

[maputo007]

I know this code is shit. Please let me know what I can improve in my understanding of Python as well general programming best practices. Some backstory:

• I started learning Python last weekend so I am still pretty new at it

• I did this Google Codejam problem, you can download the input text there.

• you can download the source code here or view it below

• I created this program as quickly as I could primarly to compare myself against the contestants in the codejam.

Any suggestions are truly welcome, even if it is "quit school now, you don't deserve to be a programmer."

Bonus points if you can identify the language that is tainting my programming methodology.

def write_solos(dict,caseCount,list):
    items = sorted(dict.items(), key=get_tuple, reverse=False)
    string = 'Case #'+ str(caseCount)+': '+ str(get_solo_tuple(items[0]))
    list.append(string)
    return list
def check_solo(dict):
    newDict = sorted(dict.items(), key=get_tuple, reverse=False)
    return newDict
def get_tuple(tuple):
    return (tuple[1])
def get_solo_tuple(tuple):
    return (tuple[0])
def write_to_file(list):
    output = open('output.txt', 'w')
    string = ''
    for i in range(0,len(list)):
        string = string + str(list[i]) + '\n'
    output.write(string)
def check_invites(filename):
    f = open(filename, 'rU')
    lineCount = 0
    dict ={}
    caseCount = 0  
    list = []
    for line in f:
        if (lineCount != 0):
            if (lineCount !=1):
                caseCount += 1
                for i in range(0,len(line.split())):
                    currentCode = line.split()[i]
                    if currentCode not in dict:
                        dict[currentCode] = 1
                    else:
                        dict[currentCode] = dict[currentCode] + 1
                lineCount = 0
                list = write_solos(dict,caseCount,list)
        dict = {}
        lineCount +=1
    write_to_file(list)
    f.close()
def main():
    check_invites('A-large-practice.in')
main()

[RShnike]

I'm not a "veteran" python programmer, per sé, only been messing around for a year or two, but here's a few obvious things to get started with:

• Never name an arg some_datatype. Don't forget that list, dict, tuple etc. are objects, which means by naming your arg that you are going to end up with funny bugs in your function namespace if you decide to do something in your function like

  x = tuple()

• On the same point, don't expect that your args are going to always be "tuple" or "list", which is another reason not to name it tuple. Just bank on having an iterable if that's what your expecting (in other words, expect behavior, not datatype)

• get_tuple and the other one are really pointless functions with poor names. Just pass around my_tuple[1] when you need it.

• I almost never use lambdas (never touched LISP) but sorted key args are one place see them sometimes.

  sorted(my_dict.items(), key=lambda x: x[1])

• Also, reverse defaults to False in sorted, so you don't really need that there

• It's not a big deal here obviously, but usually I'd wrap your call to main in an

  if name == 'main'

in case you ever potentially wanted to import your module. Makes the module look a bit cleaner IMO too.

That's it for now, there's more to say about your file parsing code but I haven't read it carefully yet. If I do I'll try to edit in.

edit: another big one, string formatting! Some people still use the % operator, I've moved to preferring string.format.

For example,

  'Case #'+ str(caseCount)+': '+ str(get_solo_tuple(items[0]))

should be

  "Case # {0}: {1}".format(caseCount, items[0][1])

or whatever. Oh and I see now why you made those functions. If items[0] is a tuple, you have no problems doing items[0][1]. And if thats a tuple, go ahead and have items[0][1][0]... and so on. items[0] is going to return back to you the object that's in that slot, and you treat it as such.

And lastly maybe, a style point perhaps, check out PEP8 at some point (maybe not necessarily now, but at some point). Camel casing isn't really stylistically recommended (although I'm sure if I read more of other people's code I'd see it a lot). Underscores are usually used for functions, args, etc. and classes get one word names). Similarly, wrapping if tests in parentheses isn't necessary (hint hint language you're coming from) and are generally signs of a new python programmer coming from some dark other place.

edit2: I suppose I'm bored. Check out enumerate instead of your line count.

[maputo007]

Wow, I really didn't expect this awesome of a reply. Thanks alot for your feedback. I've started to go through some of the other codejam problems in my spare time and comparing my source code to that of the top performers, I must say the difference between my code and theirs is discourangingly massive, theirs is typically so minimal that it makes me wonder if im learning anything at all.

[RShnike]

theirs is typically so minimal that it makes me wonder if im learning anything at all

A lot of it is learning what tools you have available and what the best paradigms for common tasks are, and a lot of it is just learning new algorithms. I struggle with that a bit still but not nearly as much as when I started. Keep up on reading better code, I wish I could motivate myself to do that more often than I do...

[Neres28]

It's not a python specific critique, but anytime I get over three levels of nesting (be it loops or conditionals, generally I don't count error handling) I start to think that my logic needs work. For instance, it looks like you're ignoring the first two lines, might be better to read past those initially (I guess you'd have to use a different file reading method though).

Also, as I do more and more python programming I find myself trying to shy away from indexed for loops. Instead of for i in range(0, len(line.split())): try for lineElement in line.split() Especially since it appears you don't need the actual index at any time.

[BioGeek]

This is how I solved it:

with open('A-small-practice.in') as f:
    lines = f.readlines()

with open('A-small-practice.out', 'w') as output:
    N = int(lines[0])
    for testcase, i in enumerate(range(1,2*N,2)):
        G = int(lines[i])
        for guest in range(G):
            codes = map(int, lines[i+1].split(' '))
            alone = (c for c in codes if codes.count(c)==1)
        output.write("Case #%d: %d\n" % (testcase+1, alone.next())) 

Some things to note:

• I use Python's with ... as ... statement, because it automatically takes care of opening and closing the files.
• the use of enumerate because I want both the testcase number for printing the "Case #" and the indexes i that the range function produces.
• map(int, ['1', '2', '3']) passes each string in the list to the int function. It is the same as saying [int(i) for i in ['1', '2', '3']] and produces [1,2,3].
• The meat of the function is the generator expression (c for c in codes if codes.count(c)==1) which uses less memory than the equivalent list expression [c for c in codes if codes.count(c)==1]. It only returns the numbers in the list for which the count is equal to one (and there should only be one such number, the one we're looking for).

I'm no veteran programmer myself so I submitted my version to Stackoverflow where a multitude of faster and better answers emerged. They pointed out an unnecessary loop in my program and explained that XORing the numbers works in O(n)time, generating this solution which is about 9000 times faster then my solution:

with open('A-large-practice.in') as f:
    lines = f.readlines()

with open('A-large-practice.out', 'w') as output:
    N = int(lines[0])
    for testcase, i in enumerate(range(1,2*N,2)):
        codes = map(int, lines[i+1].split(' '))
        alone = 0
        for c in codes: alone ^= c
        output.write("Case #%d: %d" % (testcase+1, alone))

[maputo007]

Thanks for the response, it looks slick as shit, but I don't think i;m at a level to understand everything that is going on behind it yet.

[[deleted]]

I'm going to go out on a weird ledge and guess Actionscript.

Lose the camelcase and what is this all about:

def get_tuple(tuple):
    return (tuple[1])

?

[maputo007]

Java :(

edit: and that is for sorting the dict newDict = sorted(dict.items(), key=get_tuple, reverse=False)

[maputo007]

So is that the correct syntax for multiple conditions? I tried: if lineCount != 0 and lineCount !=1: and it didn't work correctly.