Path: csiph.com!fu-berlin.de!uni-berlin.de!not-for-mail
From: Chris Angelico <rosuav@gmail.com>
Newsgroups: comp.lang.python
Subject: Re: Brief Code Review Please - Learning Python
Date: Mon, 7 Dec 2015 04:49:15 +1100
Lines: 48
Message-ID: <mailman.3.1449424163.2247.python-list@python.org>
References: <8fce9c71-a7bf-43fa-b8cc-f23c268e2b05@googlegroups.com> <CAPM-O+xKvgr4FLRNWyWWTDEK6Z5-4vt47+b_7Vvq6Pz02F_Nfg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
In-Reply-To: <CAPM-O+xKvgr4FLRNWyWWTDEK6Z5-4vt47+b_7Vvq6Pz02F_Nfg@mail.gmail.com>
Precedence: list
Xref: csiph.com comp.lang.python:100059

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