current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

First some background - I am developing a Python program which has thousands of lines spread across many files. I have intermediate programming skills - and no commercial OOP experience, and have taught myself Python recently. I have come across a problem which I can solve, in a number of different ways, but I'm not really happy with any of my solutions - they don't feel quite right from an OOP point of view. I have tried to make things generic so they can be used for lots of different options, but by doing so it seems massively convoluted.

I have distilled the problem down into a comparatively small piece of code for review. I have a main object, and I have an object which holds some options (which are just true/false for now, but could be other things in the future). I then have a third object (a tkinter button) that each show the status of the option (via being sunken or raised), and when clicked, change the value.

I felt the right way to do this would be for the functionality (in italics above) to be contained entirely in the toggleButton class. At first I just passed it self.options.Option1 (or whatever), but that didn't work. I then read up on does Python pass by value or reference, which (I know I am simplifying here) seemed to say that when you pass an object it passes the object, but when you pass something like an integer or boolean it passes the value. So I changed it to what you see now - where you pass the variable, an integer representing the variable, and changing the options class to have a function to change the option. It all seems so needlessly verbose. Is there a better way?

from Tkinter import *

class optionsClass():
    _OPTION1 = 1
    _OPTION2 = 2

    def __init__(self):
        self.Option1 = True
        self.Option2 = False

    def change(self, optionid):
        if optionid == optionsClass._OPTION1:
            if self.Option1 == True:
                self.Option1 = False
            else:
                self.Option1 = True
        if optionid == optionsClass._OPTION2:
            if self.Option2 == True:
                self.Option2 = False
            else:
                self.Option2 = True

class toggleButton(Button):
    def __init__(self, master, targetObject, targetoptionid, initialval
                    , **kwargs):
        Button.__init__(self, master, command = self.callback, **kwargs)
        self.targetoptionid = targetoptionid
        self.targetObject = targetObject
        if initialval==True:
            self.config(relief=SUNKEN)
        else:
            self.config(relief=RAISED)

    def callback(self):
        self.targetObject.change(self.targetoptionid)
        if self.cget("relief") == SUNKEN:
            self.config(relief=RAISED)
        else:
            self.config(relief=SUNKEN)


class mainframe(Frame):
    def __init__(self, master):
        Frame.__init__(self, master)
        self.options = optionsClass()
        option1box = toggleButton(self, self.options, optionsClass._OPTION1,
                self.options.Option1, text="Option 1")
        option1box.grid(column=0, row=0)
        option2box = toggleButton(self, self.options, optionsClass._OPTION2,
                self.options.Option2, text="Option 2")
        option2box.grid(column=0, row=1)
        testbutton= Button(self, command = self.outputoptions,
                             text="Return option values")
        testbutton.grid(column=0, row=2)

    def outputoptions(self):
        print "Option 1 is " + str(self.options.Option1)
        print "Option 2 is " + str(self.options.Option2)


top = Tk()
m = mainframe(top)
m.pack()
top.mainloop()

Things to bear in mind...

  • I would like the options class to work for things other than boolean without much difficulty.
  • I like the convenience and readability of the main class to simply say self.options.Option1.
  • Ideally there will still be three classes, as mainframe and option do a lot more in the actual program, and toggleButton is used multiple times and is therefore a good candidate to be a class.
share|improve this question

2 Answers 2

up vote 4 down vote accepted

First, to answer your question on passing by value/reference, Python doesn't pass mutable (e.g. list, dict) and immutable (e.g. str, tuple) objects any differently - the difference appears because they are[n't] mutable, i.e. can[not] be changed in-place. Names in Python are just references to objects.

When you pass an immutable object:

def calling():
    foo = "bar" # bind str object to name 'foo' in scope of 'calling'
    called(foo) # pass reference to str object to 'called'
    print(foo) # still "bar"

def called(baz): # first arg object will be bound to name 'baz' in scope of 'called'
    baz = "{0}!".format(baz) # rebinds name 'baz' to **new str object**
    print(baz)

In this case, the string is immutable, it can't be changed in-place, so changes in called won't be reflected in calling, as calling still references the original object.

By contrast, with a mutable type:

def calling():
    foo = [] # bind list object to name 'foo' in scope of calling function
    called(foo) # pass reference to list object to 'called'
    print(foo) # see change from 'called'

def called(baz): # first arg object will be bound to name 'baz' in scope of 'called'
    baz.append("hello, world") # object bound to name 'baz' (and 'foo') **is changed**
    print(baz)

You should read and consider following the Python Style Guide, PEP-0008 - class names should be CapitalizedWords and variable, function and method names should be lowercase_with_underscores.

optionsClass can be streamlined. At the very least, note that

if self.Option1 == True:
    self.Option1 = False
else:
    self.Option1 = True

can be simplified to:

self.Option1 = not self.Option1

Dictionaries would allow you to simplify further:

class OptionsClass():

    id_map = {'Option1': 1, 'Option2': 2}

    def __init__(self, options=None):
        if options is None:
            options = {1: True, 2: False}
        self.options = options

    def change(self, option_id):
        self.options[option_id] = not self.options[option_id]

    def __getattr__(self, attr_name):
        return self.options[id_map[attr_name]]

Now you can use it:

>>> o = OptionsClass()
>>> o.Option1
True
>>> o.Option2
False
>>> o.change(1)
>>> o.Option1
False
share|improve this answer
    
Thanks, I haven't gotten fully used to the ideas of dicts yet, great chance to practice the use of it here so thanks. –  WelshGandalf Jun 4 '14 at 23:13
up vote 3 down vote

@jonrsharpe did most of the heavy lifting on this answer. I'll just point out a couple things that can be improved:

  1. In the statement if self.Option1 == True, the == True part is not needed. Since self.Option1 holds a boolean value, you can simply use its value to satisfy the if's conditional.

    As a side-note, you can actually use non-boolean values as the conditional in an if-statement because Python treats empty iterables as False. So:

    foo = []
bar = {}
baz = ''

if not foo and not bar and not baz:
    print('They all were considered False')
else:
    print('Guess I was wrong...')

  2. Don't use from MyModule import * unless you really are using everything from that module. If you aren't using everything, then the star-import can bring a lot of unnecessary fluff into your program.

    A general rule-of-thumb is to be as specific as prudent.

    Example: say I use a lot of QWidgets from PySide.QtGui. I'm not going to import them all:

    from Pyside.QtGui import QTableWidget, QPushButton, QLabel, etc...

    because, odds are, I'll will eventually need another QWidget (or stop using one). This clutters up the top of the file and can make it hard to maintain. Instead, I would simply use the import statement:

     from PySide import QtGui

    and append the QtGui namespace to all of my QWidget declarations.

  3. As Jon's answer pointed out, take a look at PEP8. It will help your code look and feel more Pythonic. Currently your code is quite good. However, there are a few nitpick-ey things I could find:

    • Don't put spaces before and after the = sign for keyword arguments.
    • Use a single blank line to separate logical sections of code. This can help your mainFrame's __init__ function look better.
    • Use 2 blank lines between class declarations.
share|improve this answer
    
Using var == True or simply using var... I knew about this but 1) I find the former more readable, and 2) the code I am basing it on uses == True, so I stuck with it. :) About importing... I am indeed using an absolute ton of widgets... does the benefit of only importing some outweigh the convenience/readability of my code? And thanks I will read PEP8 and try to abide by it - I want to put this code on github eventually so I want it to be as Pythonic as possible. –  WelshGandalf Jun 4 '14 at 23:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.