Brief Code Review Please - Learning Python

[Peter Otten]

qualityaddictorg at 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))