Getting OT here -- you've been warned.
On Sun, Feb 14, 2021 at 2:10 AM Rob Cliffe <rob.cli...@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
_______________________________________________
Python-ideas mailing list -- python-ideas@python.org
To unsubscribe send an email to python-ideas-le...@python.org
https://mail.python.org/mailman3/lists/python-ideas.python.org/
Message archived at
https://mail.python.org/archives/list/python-ideas@python.org/message/ENAHOCYLAKMYA6X7YUDDCAKDA64BTQJF/
Code of Conduct: http://python.org/psf/codeofconduct/
