Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #100059

Re: Brief Code Review Please - Learning Python

Show key headers only | View raw

On Mon, Dec 7, 2015 at 4:34 AM, Joel Goldstick <joel.goldstick@gmail.com> wrote:
>>  5      if (desired_text != '') and \
>>  6         (desired_font_family != '') and \
>>  7         (desired_font_size != '') and \
>>  8         ((is_multi_lines == "True") or (is_multi_lines == "False")):
>>
>
> The above test will always be True.  Look up what is considered True in
> Python, and what is False.  I imagine you don't want quotes around True and
> False.  Any string will == True

Not sure what you mean by this. If you're talking about boolification,
then an empty string counts as false, but that's not what's happening
here. It's checking that the string be equal to one of two other
strings - and Python behaves exactly the way any sane person would
expect, and this condition is true only if the string is exactly equal
to either "True" or "False".

But I think this line of code should be unnecessary. It's guarding the
entire body of the function in a way that implies that failing this
condition is an error - but if there is such an error, the function
quietly returns None. Instead, I would go for a "fail-and-bail" model:

def measure_string(text, font_family, font_size, multiline):
    if not text:
        raise ValueError("Cannot measure empty string")
    if not font_family:
        raise ValueError("No font family specified")
    if multiline not in ("True", "False"):
        raise ValueError("Must specify 'True' or 'False' for multiline flag")
    ... body of function here ...

(Incidentally, it's quite un-Pythonic to use "True" and "False" as
parameters; if the function you're calling requires this, it might be
worth papering over that by having *your* function take the bools True
and False, and converting to the strings on the inside.)

> 16          WidthAndHeight = namedtuple("WidthAndHeight", "Width Height")

You're creating a brand new namedtuple type every time you enter this
function. That's expensive and messy. I would recommend either (a)
constructing one "Dimension" type, outside the function, and using
that every time; or (b) returning a regular tuple of width,height
without the names. The convention of width preceding height (or x
preceding y, more generally) is sufficiently widespread that it's
unlikely to confuse people.

ChrisA

Back to comp.lang.python | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Brief Code Review Please - Learning Python qualityaddictorg@gmail.com - 2015-12-06 09:21 -0800
  Re: Brief Code Review Please - Learning Python Joel Goldstick <joel.goldstick@gmail.com> - 2015-12-06 12:34 -0500
    Re: Brief Code Review Please - Learning Python Thomas 'PointedEars' Lahn <PointedEars@web.de> - 2015-12-07 21:21 +0100
  Re: Brief Code Review Please - Learning Python Chris Angelico <rosuav@gmail.com> - 2015-12-07 04:49 +1100
  Re: Brief Code Review Please - Learning Python Peter Otten <__peter__@web.de> - 2015-12-06 19:14 +0100

csiph-web