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

Groups > comp.lang.python > #100057 > unrolled thread

Brief Code Review Please - Learning Python

Back to article view | Back to comp.lang.python

Contents

  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

#100057 — Brief Code Review Please - Learning Python

[Note: Tried sending this twice to the Python-List mail list but I never saw it come through to the list.]

Hello everyone,

    I am beginning to learn Python, and I've adapted some code from Google into the function below.  I'm also looking at the PEP 8 style guide.  I'd appreciate a brief code review so I can start this journey off on a solid foundation.  :)

    * I've already changed my editor from tabs to 4 spaces.
    * I've also set my editor to alert me at 72 characters and don't exceed 79 characters.
    * I've named the function and variables with lower case, underscore separated, and meaningful words.
    * I've surrounded this function with two blank lines before and after.
    * I just recognized I need to pick a quote style and stick to it so I'll fix that (" " or ' ').
    * I'm not sure about the WidthAndHeight on lines 16 and 17 regarding capitalization.
        If I understand correctly a namedtuple is a class so the CapWords convention is correct.
    * Is how I've aligned the function arguments on lines 1-4 and 22-25 good style or \
        is spreading these out onto fewer lines preferred?
    * Same question as right above but with the if tests on lines 5-8.
    * I've also used ( ) around the if tests, but I'm not sure if this is good Python style or not.

 1  def measure_string(desired_text,
 2                     desired_font_family,
 3                     desired_font_size,
 4                     is_multi_lines):
 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")):
 9          with Image(filename='wizard:') as temp_image:
10              with Drawing() as measure:
11                  measure.font_family = desired_font_family
12                  measure.font_size = desired_font_size
13                  measures = measure.get_font_metrics(temp_image,
14                                                      desired_text,
15                                                      multiline=is_multi_lines)
16          WidthAndHeight = namedtuple("WidthAndHeight", "Width Height")
17          width_and_height = WidthAndHeight(measures.text_width, \
18                                            measures.text_height)
19          return width_and_height
20
21
22  print(measure_string("some text\na new line",
23                       "Segoe UI",
24                       40,
25                       "True"))

Any and all feedback is much appreciated.  As I said, I'm just beginning to learn Python and want to start off with a solid foundation.  Thank you to everyone in advance for your time and thoughts.

  Jason

[toc] | [next] | [standalone]

#100058

On Sun, Dec 6, 2015 at 12:21 PM, <qualityaddictorg@gmail.com> wrote:

> [Note: Tried sending this twice to the Python-List mail list but I never
> saw it come through to the list.]
>
> Hello everyone,
>
>     I am beginning to learn Python, and I've adapted some code from Google
> into the function below.  I'm also looking at the PEP 8 style guide.  I'd
> appreciate a brief code review so I can start this journey off on a solid
> foundation.  :)
>
>     * I've already changed my editor from tabs to 4 spaces.
>     * I've also set my editor to alert me at 72 characters and don't
> exceed 79 characters.
>     * I've named the function and variables with lower case, underscore
> separated, and meaningful words.
>     * I've surrounded this function with two blank lines before and after.
>     * I just recognized I need to pick a quote style and stick to it so
> I'll fix that (" " or ' ').
>     * I'm not sure about the WidthAndHeight on lines 16 and 17 regarding
> capitalization.
>

You don't need the parens.  If you feel it is easier to understand with
them, they won't hurt


>         If I understand correctly a namedtuple is a class so the CapWords
> convention is correct.
>     * Is how I've aligned the function arguments on lines 1-4 and 22-25
> good style or \
>         is spreading these out onto fewer lines preferred?
>

If the parameters fit on one line, I think that is more common


>     * Same question as right above but with the if tests on lines 5-8.
>     * I've also used ( ) around the if tests, but I'm not sure if this is
> good Python style or not.
>
>  1  def measure_string(desired_text,
>  2                     desired_font_family,
>  3                     desired_font_size,
>  4                     is_multi_lines):
>  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


>  9          with Image(filename='wizard:') as temp_image:
> 10              with Drawing() as measure:
> 11                  measure.font_family = desired_font_family
> 12                  measure.font_size = desired_font_size
> 13                  measures = measure.get_font_metrics(temp_image,
> 14                                                      desired_text,
> 15
> multiline=is_multi_lines)
>

No need to set multiline = ... when is_multiline is already defined


> 16          WidthAndHeight = namedtuple("WidthAndHeight", "Width Height")
> 17          width_and_height = WidthAndHeight(measures.text_width, \
> 18                                            measures.text_height)
> 19          return width_and_height
> 20
> 21
> 22  print(measure_string("some text\na new line",
> 23                       "Segoe UI",
> 24                       40,
> 25                       "True"))
>
> Any and all feedback is much appreciated.  As I said, I'm just beginning
> to learn Python and want to start off with a solid foundation.  Thank you
> to everyone in advance for your time and thoughts.
>
>   Jason
> --
> https://mail.python.org/mailman/listinfo/python-list
>



-- 
Joel Goldstick
http://joelgoldstick.com/stats/birthdays

[toc] | [prev] | [next] | [standalone]

#100104

Joel Goldstick wrote:

> On Sun, Dec 6, 2015 at 12:21 PM, <qualityaddictorg@gmail.com> wrote:
>>     * Same question as right above but with the if tests on lines 5-8.
>>     * I've also used ( ) around the if tests, but I'm not sure if this is
>> good Python style or not.
>>
>>  1  def measure_string(desired_text,
>>  2                     desired_font_family,
>>  3                     desired_font_size,
>>  4                     is_multi_lines):
>>  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.

How did you get that idea?

> Look up what is considered True in Python, and what is False.

ISTM that *you* should do that.

> I imagine you don't want quotes around True and False.

I imagine also that they do not want to write “== True” and “== False”.  
Because you can omit the former and should replace “x == False” with
“not x”.  This applies to many programming languages, even those that do 
type conversion (and have “!” instead of “not”).

> Any string will == True

No, Python does not do type conversion on comparison by default.

$ python -c 'print("" == False)'
False

$ python -c 'print("True" == True)'
False

$ python -c 'print(True == True)'
True

(You have to define the __eq__() method of an object for that, and then you 
should also define at least __ne__().)

>>  9          with Image(filename='wizard:') as temp_image:
>> 10              with Drawing() as measure:
>> 11                  measure.font_family = desired_font_family
>> 12                  measure.font_size = desired_font_size
>> 13                  measures = measure.get_font_metrics(temp_image,
>> 14                                                      desired_text,
>> 15
>> multiline=is_multi_lines)
>>
> 
> No need to set multiline = ... when is_multiline is already defined

Watch for word wrap.  He sets the argument for the “multiline” parameter in 
the measure.get_font_metrics() method call from the “is_multi_lines” 
parameter of the measure_string() call.

Please trim your quotes to the relevant minimum.

OP: Please do not post code line numbers or any other "decoration" if you 
want a code review.  Especially with Python it is important that you post 
the code verbatim.

-- 
PointedEars

Twitter: @PointedEars2
Please do not cc me. / Bitte keine Kopien per E-Mail.$

[toc] | [prev] | [next] | [standalone]

#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

[toc] | [prev] | [next] | [standalone]

#100060

qualityaddictorg@gmail.com wrote:

> [Note: Tried sending this twice to the Python-List mail list but I never
> [saw it come through to the list.]
> 
> Hello everyone,
> 
>     I am beginning to learn Python, and I've adapted some code from Google
>     into the function below.  I'm also looking at the PEP 8 style guide. 
>     I'd appreciate a brief code review so I can start this journey off on
>     a solid foundation.  :)
> 
>     * I've already changed my editor from tabs to 4 spaces.
>     * I've also set my editor to alert me at 72 characters and don't
>     exceed 79 characters. * I've named the function and variables with
>     lower case, underscore separated, and meaningful words. * I've
>     surrounded this function with two blank lines before and after. * I
>     just recognized I need to pick a quote style and stick to it so I'll
>     fix that (" " or ' '). * I'm not sure about the WidthAndHeight on
>     lines 16 and 17 regarding capitalization.
>         If I understand correctly a namedtuple is a class so the CapWords
>         convention is correct.
>     * Is how I've aligned the function arguments on lines 1-4 and 22-25
>     good style or \
>         is spreading these out onto fewer lines preferred?
>     * Same question as right above but with the if tests on lines 5-8.
>     * I've also used ( ) around the if tests, but I'm not sure if this is
>     good Python style or not.
> 
>  1  def measure_string(desired_text,
>  2                     desired_font_family,
>  3                     desired_font_size,
>  4                     is_multi_lines):
>  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")):
>  9          with Image(filename='wizard:') as temp_image:
> 10              with Drawing() as measure:
> 11                  measure.font_family = desired_font_family
> 12                  measure.font_size = desired_font_size
> 13                  measures = measure.get_font_metrics(temp_image,
> 14                                                      desired_text,
> 15                                                     
> multiline=is_multi_lines)
> 16          WidthAndHeight = namedtuple("WidthAndHeight", "Width Height")
> 17          width_and_height = WidthAndHeight(measures.text_width, \
> 18                                            measures.text_height)
> 19          return width_and_height
> 20
> 21
> 22  print(measure_string("some text\na new line",
> 23                       "Segoe UI",
> 24                       40,
> 25                       "True"))
> 
> Any and all feedback is much appreciated.  As I said, I'm just beginning
> to learn Python and want to start off with a solid foundation.  Thank you
> to everyone in advance for your time and thoughts.
> 
>   Jason

Use short variable names - the "desired_" prefix does not carry much 
information.

Avoid unrelated but similar names: measure or measures -- what's what?

Use consistent names. This is a bit tricky, but I would prefer multiline 
over is_multi_lines because the underlying library uses the former.

Use the right datatype: multiline should be boolean, i. e. True or False, 
not "True" or "False".

Provide defaults if possible: if the multiline argument is missing you can 
scan the text for "\n" and set the flag accordingly

Prefer parens for line continuations

(a and
b) 

over backslashes

a and \
b

Move code that only should be executed once from the function to the global 
namespace: the namedtuple definition should be on the module level.

Use qualified names if you are referring library code: wand.drawing.Drawing 
instead of just Drawing.

Avoid spurious checks, but if you do argument validation fail noisyly, i. e.

"wrong":

def f(fontname):
    if is_valid(fontname):
        return meaningful_result()
    # implicitly return None

"correct":

class InvalidFontname(Exception):
    pass

def f(fontname):
    if not is_valid(fontname):
        raise InvalidFontname(fontname)
    return meaningful_result()


With changes along these lines your code might become


from collections import namedtuple

import wand.drawing
import wand.image


Extent = namedtuple("Extent", "width height")


def get_text_extent(
        text, font_family, font_size, multiline=None):
    """Determine width and heights for `text`.
    """
    if multiline is None:
        multiline = "\n" in text

    with wand.image.Image(filename='wizard:') as image:
        with wand.drawing.Drawing() as measure:
            measure.font_family = font_family
            measure.font_size = font_size
            metrics = measure.get_font_metrics(
                image, text, multiline=multiline)

            return Extent(
                width=metrics.text_width,
                height=metrics.text_height)


if __name__ == "__main__":
    print(
        get_text_extent(
            "some text\na new line",
            "Segoe UI",
            40))

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web