You are not logged in.

#1 2011-10-18 07:04:04

Google
Member
From: Mountain View, California
Registered: 2010-05-31
Posts: 484
Website

Could I get some help critiquing my Python code?

I recently tried out Python and so far I like it. I tried out some GUI programming with Tkinter. After my previous thread-- I thought it would be best to try out each of the GUI libraries, so Tk was my first.

I wrote two pretty simple applications: a die roller and a text editor. I could use some help critiquing my code-- I hope to learn any ways to improve my code.

Notepyad
PyRoller

Thanks for any feedback smile

Offline

#2 2011-10-18 11:16:17

lunar
Member
Registered: 2010-10-04
Posts: 95

Re: Could I get some help critiquing my Python code?

Quite ok, except for some small remarks:

Classes and files (aka modules) are independent in Python.  Unlike Java, you do not need to name the module after the contained class, and you can perfectly put more than one class in a module.  It is actually recommended to do so.  A "Die" module just to contain a "Die" class is superfluous, just put the Die class into the main script.   Concerning module names, keep to PEP 8.  "Die.py" should read "die.py".

Use the "with" statement when dealing with files. Instead of:

try:
     filepointer.write(text.rstrip())
finally:
     filepointer.close()

Just:

with filepointer:
    filepointer.write(text.rstrip())

Btw, "filepointer" is a misleading name.  There are no pointers in Python.

"save_as_flag" in "FileHandler.py" is quite ugly.  I'd have implemented saving this way:

def save(self, filename=None):
    filename = filename or self.filename
    if not filename:
        raise ValueError()
    # save the file *without* any user interaction
    text = self.text_widget.get(1.0, END)
    with open(filename, 'w') as stream:
        stream.write(text.rstrip())

def save_file(self):
    if self.filename:
        self.save()
    else:
        self.save_file_as()

def save_file_as(self):
     filename = asksaveasfilename(filetypes=FILE_TYPES)
     self.save(filename)
     self.filename = filename

You ignore encodings.  If your text-editor is run on Python 2, it'll fail to save any text that does not only contain ASCII characters.

If you indent to write source code for both, Python 2 and Python 3, use future imports to make Python 2 more like Python 3 (e.g. "from __future__ import print_function, unicode_literals" mainly).

Last edited by lunar (2011-10-18 11:16:36)

Offline

#3 2011-10-18 11:55:03

Google
Member
From: Mountain View, California
Registered: 2010-05-31
Posts: 484
Website

Re: Could I get some help critiquing my Python code?

Thanks, I will work on fixing this. I didn't know about "with" or importing from the future. I will look them up. I checked out the PEP8 style guide-- it's pretty nice. I need to get used to using lower case names for the modules. I didn't know it was recommended to build modules with multiple classes-- that sounds nicer to me.

I goofed with the filepointer-- I guess a hold over from C. I automatically assumed I was working with a file pointer from the open method. I looked it up and apparently Python returns a file object from the open function. Nice catch~

I agree the save_as_flag was pretty ugly. Prior to that, I had a lot of 'duplicate' code in the save and save_as methods. I concocted that strategy to reduce the duplication but it's certainly still pretty ugly. I will work on that too. smile

Thanks--!

Offline

#4 2011-10-18 19:03:39

Yannick_LM
Member
Registered: 2008-12-22
Posts: 142

Re: Could I get some help critiquing my Python code?

Fist, this is nice work smile

Few stylistic remarks, though.

I'd rather use  .format() or the '%' sign instead of building strings with '+':

# Ugly:
new_title_string =  APP_NAME +" (" + self.filehandler.filename + ")"
# Nice:
new_title_string = "(%s (%s)" % (APP_NAME, self.filehandler.filename)
# or:
new_title_string = "{} ({})".format(APP_NAME, self.filehandler.filename)

Also, I'd rather have __init__ defined as the first method of the class.

t's where you want to look when you want to find what attributes your class has,
or how to create an instance of it,right below doc of the class,

Something like

class Foo:
   """ Does this and that
   Has a bar to ....

   """
   def __init__(bar):
      self.bar = bar

Offline

#5 2011-10-19 15:22:54

drcouzelis
Member
From: Connecticut, USA
Registered: 2009-11-09
Posts: 4,092
Website

Re: Could I get some help critiquing my Python code?

Yannick_LM wrote:

I'd rather use  .format() or the '%' sign instead of building strings with '+':

# Ugly:
new_title_string =  APP_NAME +" (" + self.filehandler.filename + ")"
# Nice:
new_title_string = "(%s (%s)" % (APP_NAME, self.filehandler.filename)

To me, your "ugly" code looks really nice and your "nice" code looks really ugly. sad

Is your "nice" method the pythonic way of doing things or just the Yannick_LM way of doing things? I'm still new to Python, so I don't know...

Offline

#6 2011-10-19 15:27:49

lunar
Member
Registered: 2010-10-04
Posts: 95

Re: Could I get some help critiquing my Python code?

@drcouzelis:  It is the pythonic way, and not just the Yannick_LM way.  String formatting, both the old way with "%" and the new way with ".format()", is preferable over word-puzzling with "+", being more flexible and easier to read. 

I guess, your preference for "+" is a remnant of Java programming, and even in Java you'd use "String.format()" or "StringBuilder" instead of string concatenation nowadays, unless concatenating literals only (e.g. to avoid over-long lines).

Last edited by lunar (2011-10-19 15:30:27)

Offline

#7 2011-10-19 17:56:49

Google
Member
From: Mountain View, California
Registered: 2010-05-31
Posts: 484
Website

Re: Could I get some help critiquing my Python code?

Hello-- thanks for the advice.

I have taken and implemented most of it. I am curious about the future imports-- I added them above all other imports. It seems like the logical place, but I am curious if they will conflict with Python 3? I researched it a bit and it seems the future imports only work on 2.6 and greater so I tossed in a comment for that as well, but didn't check the Python version they have.

I did fix the string formatting in the title bar. I didn't know about the format method, that's pretty nifty. smile

I haven't fixed the ugly save_as flag, that's next on my list.

Thanks-- learning a lot.

Offline

#8 2011-10-19 18:20:59

lunar
Member
Registered: 2010-10-04
Posts: 95

Re: Could I get some help critiquing my Python code?

"__future__" imports are ignored on Python versions which already provide the corresponding feature by default.  Thus a "from __future__ import print_function" won't do any harm when run with Python 3.  It just doesn't have any effect, as "print" being a function is already the default on Python 3.

Offline

#9 2011-10-20 18:41:49

Google
Member
From: Mountain View, California
Registered: 2010-05-31
Posts: 484
Website

Re: Could I get some help critiquing my Python code?

Thanks!

I finally knocked out that save as flag-- I also tested unicode. All seems to work well.

I fixed a bug too-- I made sure the editor is cleared of any text before it opens and displays a file's contents.

Offline

#10 2011-10-21 02:13:52

Nisstyre56
Member
From: Canada
Registered: 2010-03-25
Posts: 85

Re: Could I get some help critiquing my Python code?

drcouzelis wrote:
Yannick_LM wrote:

I'd rather use  .format() or the '%' sign instead of building strings with '+':

# Ugly:
new_title_string =  APP_NAME +" (" + self.filehandler.filename + ")"
# Nice:
new_title_string = "(%s (%s)" % (APP_NAME, self.filehandler.filename)

To me, your "ugly" code looks really nice and your "nice" code looks really ugly. sad

Is your "nice" method the pythonic way of doing things or just the Yannick_LM way of doing things? I'm still new to Python, so I don't know...


Using + is less efficient, and less readable.

see: http://www.joelonsoftware.com/articles/ … 00319.html

I did a small test:

#! /usr/bin/env python2

string1 = "".join([str(i) for i in xrange(1,10000)])
string2 = "".join([str(i) for i in xrange(1,10000)])
string3 = "".join([str(i) for i in xrange(1,10000)])
string4 = "".join([str(i) for i in xrange(1,10000)])
string5 = "".join([str(i) for i in xrange(1,10000)])
string6 = "".join([str(i) for i in xrange(1,10000)])

def concatstring():
    return string1+string2+string3+string4+string5+string6

def formatstring():
    return "%s%s%s%s%s%s" % (string1, string2, string3, string4, string5, string6)


if __name__ == '__main__':
    from timeit import Timer
    t = Timer("concatstring()", "from __main__ import concatstring")
    t2 = Timer("formatstring()", "from __main__ import formatstring")
    print t2.timeit(),t.timeit()

which gives...

>python2 -u "string_efficiency.py"
50.7550070286 89.0668880939
>Exit code: 0

What that is saying is that the first took 50 seconds, and the second took 89 seconds. the timeit module will take the best out of several runs. Remember, t = 89 = concatstring, and t2 = 50 = formatstring.

In order to understand why this is, you must first understand that in Python, strings are immutable. So when you add a string, you run the entire length of the string, and then add on the new string, and return a new string.

For example, in order to evaluate "foo"+"bar"+"baz"+"quux" you would do...

"foo"+"bar" = "foobar"
"foobar" + "baz" = "foobarbaz"
"foobarbaz" + "quux" = "foobarbazquux"

Notice that the length of the string gets longer each time, and we have to traverse "foo" 3 times in order to concatenate it with the rest of the strings.

When you use string formatting, you create the string all in one go. Which is much more efficient.


Edit:

Also, 30 extra seconds may not seem like much longer, but when you start concatenating a much greater number of strings, they will be vastly different in terms of performance, in fact, if I do the same thing as above with 18 strings instead of 6, I get:

>python2 -u "string_efficiency.py"
9.79696917534 25.3364560604
>Exit code: 0

Which is a *huge* difference.

Edit2:

I've been informed that += is more efficient in later versions of python due to cleverness on the part of the interpreter/compiler, but, for example doing foo = foo + bar in a loop is still bad.

Last edited by Nisstyre56 (2011-10-21 20:57:56)


In Zen they say: If something is boring after two minutes, try it for four. If still boring, try it for eight, sixteen, thirty-two, and so on. Eventually one discovers that it's not boring at all but very interesting.
~ John Cage

Offline

#11 2011-10-21 13:27:39

Google
Member
From: Mountain View, California
Registered: 2010-05-31
Posts: 484
Website

Re: Could I get some help critiquing my Python code?

Thanks-- I will keep  that in mind and try to use format strings. smile

Offline

#12 2011-10-21 14:48:46

steve___
Member
Registered: 2008-02-24
Posts: 452

Re: Could I get some help critiquing my Python code?

@Nisstyre56 - Do you mind shooting me an email?

Offline

Board footer

Powered by FluxBB