Getting OT here -- you've been warned.

On Sun, Feb 14, 2021 at 2:10 AM Rob Cliffe <rob.cliffe@btinternet.com> wrote:

You've broken a number of "rules" code code formatting there ;-)

Thanks for the quotation marks.  Indeed, PEP 8 provides guidelines, not diktats.

A big one is aligning things vertically, which, as a rule, I like. but this example does really push things out away from each other, so I'm not so sure.

It's written to suit my editor.  I'm with Brendan: restricting the whole world to 80 chars per line is like insisting the whole world program in COBOL.

I agree about the 80 char limit -- personally using usually 90 or 95.

But I do think you need to consider not just your editor -- if anyone else is going to read your code.

But in your example, it not only has long lines, but by lining up the stuff after the colon -- there is a LOT of info way over on the right, which I'm not so sure about.

Just for fun, here it is after being run through Black (which I don't usually use ....) -- but I think it looks better that way.

Also -- my first thought looking at that was that it could really benefit from the pattern matching :-)

Yes IMHO it would be a slight improvement.  But AFAIU pattern matching is not available yet.

no, it's not. That was a note for me in a way -- I'm a bit of a skeptic about pattern matching, so this is a good example use case.

 

And my second thought was that there could be a whole other pattern used here -- I usually find long chained elifs can be better expressed as a dict selection, or subclassing, or ...

I cannot imagine how a dictionary, much less a subclass (of what)?  could have any relevance here.  I'd be interested if you could elucidate.

Again, I don't know what the rest of your code looks like, so maybe none of these would work well, but:

for using a dict, you could set it up something like:

ch_proc_dict = {"B": self.ToggleBackgroundColour,

                "C": self.ChangeCommonChars,

                "D": self.DeleteArea,

                ...

                }

then:

if process_dict[ch](event):

    return

you also have other checks in there, so those would have to be moved into the functions in the dict, maybe with wrappers (or not -- depends on where you store some of that state data.

as for subclassing, that's a very different pattern that probably doesn't apply here now that I think about it:

This looks to me like wxPython code (or the like).

  for wx.lib.floatcanvas, I don't have keyboard accelerators, but I do have "GUI Modes"  classes: Depending on what Mode is set at the moment, each event gets mapped to different actions, so you might have different handling of the character hit depending on what object is selected:

object_selected.process_keystroke(ch)

Of course, you'd need this kind of "switch" construct in the object class anyway. Unless you wanted to get really wordy, and have:

def process_A(self..)

    ...

def process_B(self...)

    ...

you could be a bit saved in Python:

getattr(selected_object, "process" + ch)(event)

Anyway, as I say -- pretty darn off topic, but those are two of the three ways that I think "switch -- case" can be spelled in Python.

-Chris B

     if ch == "B":

    self.ToggleBackgroundColour(event)
elif ch == "C":
    self.ChangeCommonChars(event)
    return
elif ch == "D" and len(Areas) > 1:
    self.DeleteArea(event)
    return
elif ch == "F" and __debug__:
    self.Info(event)
    return
elif ch == "I":
    self.ChangeImagesToScroll(event)
    return
elif ch == "J" and self.NUMBER_OF_IMAGES == 1:
    self.ChangeJustification(event)
    return
elif ch == "L":
    self.ToggleShowLabels(event)
elif ch == "M" and MaxNumberOfImages > 1:
    self.ChangeNumberOfImages(event)
elif ch == "N" and CanZapImage:
    self.Rename(event)
elif ch == "R" and not (area.Dynamic | area.IsTour):
    self.ToggleRandom(event, 1)
elif ch == "S":
    self.ToggleSets(event, 1)
elif ch == "T":
    self.ChangeTimer(event)
    return
elif ch == "U" and not (area.Dynamic | area.Random):
    self.ToggleIsTour(event, 1)
elif ch == "V" and OnAnImage:
    self.WindowsPhotoViewer(event)
    return
elif ch == "W":
    self.ToggleWrap(event, 1)
elif ch == "Y" and not (area.Random | area.IsTour):
    self.ToggleDynamic(event, 1)
elif ch == "Z" and CanZapImage:
    self.Zap(event)
elif ch == "A" and not (area.Dynamic | area.IsTour):
    self.ToggleRandom(event, 1)
# Alt-R doesn't always work for some reason, so we give Alt-A as an alternative
else:
    event.Skip()

I'm well aware that if you apply PEP 8 guidelines strictly it would look like the above.  But is this supposed to be an improvement?

I did it to compare and decide -- I'm ambivalent.

 

  My original was a clear, tabular layout making the code structure completely clear, the vertical alignment highlighting where different cases are treated differently and where they are treated similarly.  This is just an amorphous mass of code that I find much harder to grok.  (YMMV but I find it hard to imagine.)

I find it pretty clear -- the pattern and indentation make it just as clear to me that it's a repeated pattern, and the indented form makes it easier for me to match the action to the  ch selected.

PS Can anyone shed any light on why Alt-R doesn't always "work" on Windows 10?  I believe there's some bit of installed software that has hijacked it, but I'm not sure what.

I'm hopeless there ....

-Chris B

--

Christopher Barker, PhD (Chris)

Python Language Consulting
  - Teaching
  - Scientific Software Development
  - Desktop GUI and Web Development
  - wxPython, numpy, scipy, Cython