Ревью одного скринкаста

Бобук вчера попросил комментариев на свой небольшой скринкаст. Я попробовал, было, делать это прямо в Твиттере, но быстро осознал кривость затеи. Переделываю по-правильному, как нормальный ревью кода. Как в любом ревью, комментарии не претендуют на абсолютную истину и не являются оценкой автора кода — это просто взгляд другого программиста.

Полный исходник для справки.

#!/usr/bin/env python
# -*- coding: utf-8 -*-

(note) Многие таскают все эти -*- за собой копипастом, кто-то наизусть помнит. Однако Питону они на самом деле не нужны, достаточно просто coding: utf-8.

from __future__ import print_function, division, absolute_import
from PIL import Image as pImage
import numpy

import os
import random

(nit) По PEP 8 импорты стандартной библиотеки должны идти раньше третьесторонних модулей.

Практика импорта чего-то "на всякий случай" сомнительна, так как замусоривает код (division и, наверное, absolute_import тут, в общем, не нужны).

Также, вместо импорта print_function можно было, кажется, просто явно писать под Python 3.

class Image:
    """Take an information from image file"""

(grammar) """Get information from image file"""

    def __init__(self, filename):
        self.filename = filename

    def load(self):

Про метод load() потом придётся помнить, что его обязательно надо вызывать сразу за конструктором, чтобы получить хоть сколько-нибудь полезный экземпляр объекта. Вообще, практика избегания вызова чего-либо интересного прямо в конструкторе имеет смысл, кажется, только в Плюсах, где не очень удачный порядок инициализации VMT. В Питоне это совершенно нормальная практика.

        img = pImage.open(self.filename)
        small = img.resize( (Image.BLOCK_SIZE, Image.BLOCK_SIZE),
                            pImage.BILINEAR )

(nit) Поскольку оригинальное изображение дальше не нужно, можно вместо заведения во второй строке новой переменной переназначить img.

        self.t_data = numpy.array(
            [sum(list(x)) for x in small.getdata()]
        )

В sum(list(x)) лишний list, потому что sum принимает любой итератор.

(style) Возможно, map(sum, small.getdata()) здесь предпочтительней.

        del img, small
        return self

del обычно бесполезен, и здесь как раз тот случай: всё, что он делает, это удаляет ссылки на локальные переменные, чтобы потом их смог собрать сборщик мусора. Но это и так само произойдёт по выходу из функции, потому что img и small локальны. Я бы сказал, что del тут даже вреден, потому что вводит читателя в заблуждение.

    def __mul__(self, other):

Очень сомнителен выбор именно __mul__ для этой операции. У меня она никак не ассоциируется с умножением. Возможно, пересечение, или разность были бы лучше. Но ещё лучше просто придумать своё имя типа "similar" или "alike" и избежать разночтений.

    def __init__(self, dirname):
        self.dirname = dirname
        self.load()

Image требовал вызывать load() вручную, ImageList делает это сам — неконсистентность API, которой бы просто не было, если бы всё делалось в конструкторах.

        self.images = \
            [Image(os.path.join(self.dirname, filename)).load() \
                for filename in os.listdir(self.dirname)
                    if filename.endswith('.jpg')]

Как Бобук много раз повторил в скринкасте, это слишком длинная строчка. Вот более удобный вариант:

files = (f for f in os.listdir(self.dirname) if f.endswith('.jpg'))
files = (os.path.join(self.dirname, f) for f in files)
self.images = [Image(f).load() for f in files]

Генераторные выражения вместо списков в первых двух строках нужны для того, чтобы итерация происходила физически только один раз в последней строке.

    def __repr__(self):
        return '\n'.join( ( x.filename for x in self.images ) )

(nit) Лишняя пара скобок под join.

    def html(self):
        res = ['<html><body>']

В CPython cоставление строк через конкатенацию (+=) на самом деле не медленней, чем этот традиционный способ с составлением списка и join'ом в конце.

        for img in self.images:
            distances = sorted([ (img * x, x) for x in self.images ])

На мой взгляд, самое большое "преступление" всего скринкаста: расчётная и презентационная логика сведены в один неделимый for внутри одной функции html(). Генерацию результатов сравнения надо вынести в отдельную функцию, а html() должна их только оформлять.

Также в самом алгоритме сразу видны неудобства: картинки сравниваются в том числе и с собой, а также все картинок серии дубликатов по разу побывают в роли референсной.

                '<img src="' + os.path.basename(x.filename) + '" width="200"/>' + str(dist)

Строчка с %-форматированием смотрелась бы проще конкатенации с её бесконечными кавычками и приведением типов:

                '<img src="%s" width="200"/>%s' % (os.path.basename(x.filename), dist)

...

               for dist, x in distances if dist < 220]

Константу 220 надо, конечно, поименовать и положить рядом с BLOCKSIZE и THRESHOLD.

Как-то так.

P.S. А вообще, чисто архитектурно, никаких классов тут конечно не нужно. Всё в единственном экземпляре и вызывается ровно один раз, поэтому классы только лишними self везде мешаются.

Комментарии: 18 (особо ценных: 1)

Glader

попробывал

(grammar) попробовал :)
Alex Ott

"-- coding: utf-8 --" полезен не только питону, но и емаксу :-)
Daemon

А -*- разве для питона ставится? Я был уверен, что это подсказка emacs'у.
pilot34

Спасибо, продолжайте в том же духе. Очень полезно.
Ilya Kutukov

Пройдусь по библиотечным тонкостям.

Смотреть директорию в данном случае проще через glob.glob('*.jpg'), хотя неочевидность названия библиотеки может запутать читателя. Но если бы расширений было бы много, то walk и listdir удобнее.

PIL стабильный, но полузаброшенный проект, хотя и был стандартом долгое время. Имеет смысл рекомендовать к использованию поддерживаемый форк Pillow.

numpy может принимать numpy.asarray(pil_image) для PIL >= 1.1.6 и Pillow
bolk

Возвращайся, Ваня, в Россию! Совсем русский забудешь! :) Попробывал он :)
Ivan Sagalaev

(grammar) попробовал :)

О, спасибо! Поправил.

"— coding: utf-8 —" полезен не только питону, но и емаксу :-)

Я знаю, поэтому комментарий мол именно для Питона. Бобук писал код в SublimeText 2.

Имеет смысл рекомендовать к использованию поддерживаемый форк Pillow.

Бобук его и рекомендовал в самом скринкасте.

numpy может принимать numpy.asarray(pil_image) для PIL >= 1.1.6 и Pillow

О, спасибо, полезно!
leventov

Макконнелл писал, что переиспользовать переменные плохо.
Ivan Sagalaev
Макконнелл писал, что переиспользовать переменные плохо.

Да, так. Но это не догма. Плохо переиспользовать пременную для другой цели. А здесь мы по сути работаем с одним объектом — картинкой — но в два шага. И значение этой переменной на первом шаге промежуточное, а вторая по сути просто апдейтит её значение. В этом случае я считаю переиспользование не только допустимым, но и вполне очевидным.

Можно было и в одну строку всё написать, на самом деле:
```
img = pImage.open(self.filename).resize((Image.BLOCK_SIZE, Image.BLOCK_SIZE), pImage.BILINEAR)
```
denizka

review понравилось, хочу еще ))

я тут из java мира и вопрос такой - а зачем тут вообще классы? Image и ImageList? ну ведь это небольшой скрипт, из пары функций. без хранения состояний. стадии обработки файлов спрятаны/разбросаны по классами (и еще в конструкторах!!). а тут фактически pipeline. ну и написать шаги pipeline в main. будет отчетливо видно что и как. или это так принято в питоне? классы на каждый чих?
Ivan Sagalaev

зачем тут вообще классы?

О том же я в P.S. написал :-). Если получится, раскрою ещё отдельным постом.

Насчёт "принято"… Скажем так, это одна из популярных парадигм, в основном в силу популярности ОО в университетском образовании. Но она никоим образом не главенствующая, многие пишут проще.
Nikita Nemkin

Особо ценный комментарий
Если уж использовать numpy, то по назначению:
```
def load(self):
    ...
    self.t_data = np.asarray(small).sum(axis=-1, dtype=np.int16)
    ...

def __mul__(self, other):
    return np.sum(np.abs(self.t_data - other.t_data) > Image.TRESHOLD)
```
в 30 раз быстрее работать будет, за вычетом времени на загрузку картинок.

Трехэтажные comprehensions лучше переписывать как обычные циклы. Строк получается чуть больше, но читать и изменять такой код будет проще.
Pavel

Ну и еще, до кучи, поправьте: "рассчётная" на "расчётная"
Ivan Sagalaev

Ну и еще, до кучи, поправьте: "рассчётная" на "расчётная"

Спасибо, поправил. Это одна из немногих ошибок, от которых всю жизнь мне не удаётся отвязаться :-).
Miroff

Спасибо, всегда приятно посмотреть как думают другие программисты.
aruseni
А ещё вместо
```
>>> l = ["a", "b", "c"]
>>> for index in range(len(l)):
...     print("%s: %s" % (index, l[index]))
```
Можно писать:
```
>>> l = ["a", "b", "c"]
>>> for index, letter in enumerate(l):
...     print("%s: %s" % (index, letter))
```
Результат тот же, а в плане производительности, потребления памяти и так далее получается лучше.
aruseni

И да, Иван, рефакторинг шикарный, спасибо большое, интересно читать! И видео отличное. :)
sashatim

Спасибо! Класное видео, всё ясно и коротко продолжайте, очень хочется побольше такого видео может из них создать видеокнигу по python учиться языку будет интереснее.

Ревью одного скринкаста

Комментарии: 18 (особо ценных: 1)

Добавить комментарий