What about this mess? Ideas to better rewrite it?

Question

This mess is working well, but if somebody has any ideas to make it look/perform better, it would be greatly appreciated!

def OnButtonClick(b, e, f="none"):
    if b == Gui["goleft"]   and e == viz.UP: do_Cam([1.475, 7.862, 10.293])
    if b == Gui["gocenter"] and e == viz.UP: do_Cam([0, 1, 52])
    if b == Gui["goright"]  and e == viz.UP: do_Cam([0, 11, 5])
    if b == Gui["godown"]   and e == viz.UP: do_Cam([0, 16, 53])

def OnSliders(POS, S):
    if S == 1:
        Gui["bars_alpha"].message(str('%.2f'%(POS)))
        CFG["BAR_alpha"] = POS
        for i in BAR_Items: BAR_Items[i].alpha(POS)
    elif S == 2:
        Gui["shps_alpha"].message(str('%.2f'%(POS)))
        CFG["SHP_alpha"] = POS
        for i in ISOS.keys(): SHAPE[i+"_SHP"].alpha(POS)
    elif S == 3:
        Gui["bars_sizes"].message(str('%.2f'%(POS)))
        CFG["BAR_scale"] = [POS, 0.15, POS]
        for i in BAR_Items: BAR_Items[i].scale(POS, BAR_Items[i].getScale()[1], POS)
    elif S == 4:
        Gui["label_alpha"].message(str('%.2f'%(POS)))
        CFG["BARTXT_alpha"] = POS
        for i in BAR_Label: BAR_Label[i].alpha(POS)
    elif S == 5:
        Gui["label_size"].message(str('%.2f'%(POS)))
        CFG["BARTXT_scale"] = [POS, POS, POS]
        for i in BAR_Label: BAR_Label[i].scale(POS, POS, POS)
    elif S == 6:
        Gui["grid_alpha"].message(str('%.2f'%(POS)))
        CFG["grid_alpha"] = POS
        [Griditema[i].alpha(POS) for i in Griditema]
        [Griditemb[i].alpha(POS) for i in Griditemb]

After following some of the initial recommendations I received, I now have:

def OnButtonClick(b, e, f="none"):
    if e != viz.UP: return
    if b == Gui["goleft"]   : do_Cam([1.475, 7.862, 10.293])
    elif b == Gui["gocenter"] : do_Cam([0, 1, 5])
    elif b == Gui["goright"]  : do_Cam([0, 1, 5])
    elif b == Gui["godown"]   : do_Cam([0, 1, 5])

def OnSliders(POS, S):
    if S == 1:
        CFG["BAR_alpha"] = POS
        for i in BAR_Items: BAR_Items[i].alpha(POS)
    elif S == 2:
        CFG["SHP_alpha"] = POS
        for i in ISOS.keys(): SHAPE[i+"_SHP"].alpha(POS)
    elif S == 3:
        CFG["BAR_scale"] = [POS, 0.15, POS]
        for i in BAR_Items: BAR_Items[i].scale(POS, BAR_Items[i].getScale()[1], POS)
    elif S == 4:
        CFG["BARTXT_alpha"] = POS
        for i in BAR_Label: BAR_Label[i].alpha(POS)
    elif S == 5:
        CFG["BARTXT_scale"] = [POS, POS, POS]
        for i in BAR_Label: BAR_Label[i].scale(POS, POS, POS)
    elif S == 6:
        CFG["grid_alpha"] = POS
        for i in Griditema: Griditema[i].alpha(POS)
        for i in Griditemb: Griditemb[i].alpha(POS)

    mydict  = {1:"bars_alpha", 2:"shps_alpha", 3:"bars_sizes", 4:"label_alpha", 5:"label_size", 6:"grid_alpha"}
    Gui[mydict[S]].message('%.2f' % POS)

This is how it ended:

def OnButtonClick(b, e, f="none"):
    if e != viz.UP: return
    if b == Gui["goleft"]    : do_Cam([1.475, 7.862, 10.293])
    elif b == Gui["gocenter"]: do_Cam([0, 1, 5])
    elif b == Gui["goright"] : do_Cam([0, 1, 5])
    elif b == Gui["godown"]  : do_Cam([0, 1, 5])

def OnSliders(POS, S):
    D = {1:"BAR_alpha", 2:"SHP_alpha", 3:"BAR_scale", 4:"TXT_alpha", 5:"TXT_scale", 6:"grid_alpha"}
    if   S == 1: CFG[D[S]] = POS; [BAR_Items[i].alpha(POS) for i in BAR_Items]
    elif S == 2: CFG[D[S]] = POS; [SHAPE[i+"_SHP"].alpha(POS) for i in ISOS.keys()] 
    elif S == 3: CFG[D[S]] = [POS, 0.15, POS]; [BAR_Items[i].scale(POS, BAR_Items[i].getScale()[1], POS) for i in BAR_Items]
    elif S == 4: CFG[D[S]] = POS; [BAR_Label[i].alpha(POS) for i in BAR_Label]
    elif S == 5: CFG[D[S]] = [POS, POS, POS]; [BAR_Label[i].scale(POS, POS, POS) for i in BAR_Label]
    elif S == 6: CFG[D[S]] = POS; [Griditema[i].alpha(POS) for i in Griditema]; [Griditemb[i].alpha(POS) for i in Griditemb]
    Gui[D[S]].message('%.2f' % POS)

Thanks for the help!

Answer 1

First thing I would do it replace the repeated code with functions.. such as

Gui["bars_alpha"].message(str('%.2f'%(POS)))
CFG["BAR_alpha"] = POS

with

def DoIt(pos): 
  Gui[pos].message(str('%.2f'%(POS)))
  CFG[pos] = POS

Then I'd replace the big if/else with a dictionary of llamdas

Answer 2

You could replace the first line of each case with a single array lookup (Gui[keys[S]]...), but frankly I don't think there's much to be gained. The code does look complex, but the complexity is real, not just an artefact of how you coded it.

Answer 3

I would follow Preet Sangha's advice with these additional improvements.

Replace

str('%.2f'%(POS))

with

'%.2f' % POS

I think it is a bad practice to use list comprehension to produce side effects.

[Griditema[i].alpha(POS) for i in Griditema]

Assuming Griditema is a dict.

for item in Griditema.itervalues():
   item.alpha(POS)

Answer 4

def OnButtonClick(b, e, f="none"):
    if e != viz.UP: return
    if b == Gui["goleft"]: do_Cam([1.475, 7.862, 10.293])
    if b == Gui["gocenter"]: do_Cam([0, 1, 52])
    ...

Answer 5

If its working well don't mess with it unless you're overhauling other things. This isn't quite so bad as to warrant overhauling on its own. However if you are going to overhaul it look at this: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html

Then put everything after a ":" on it's own line.

I agree that [Griditema[i].alpha(POS) for i in Griditema] is also bad for readability.

See if you can make the dictionary keys consistent between the arraas. "bars_alpha" and "BAR_alpha" adds unnecessary complexity. You might make other marginal improvements such as:

def OnButtonClick(b, e, f="none"):
    cam_loc = {Gui["goleft"]:   [1.475, 7.862, 10.293],
               Gui["gocenter"]: [0, 1, 52],
               Gui["goright"]:  [0, 11, 5],
               Gui["godown"]:   [0, 16, 53]}
    if e == viz.UP: 
        do_Cam(cam_loc[b])

Answer 6

I think that mess is derived by others mess in the design of the application. My suspicion is that you don't have any action object for all of those actions.