Рефакторинг одного скринкаста

В прошлом ревью одного скринкаста я написал такой "P.S.":

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

И после этого, конечно, не выдержал и решил действительно порефакторить этот код, чтобы всё было наглядно. Также я внял призывам Бобука и тоже оформил это в виде скринкаста. Больше вам скажу, я всегда считал сам процесс рефакторинга кода достаточно интересным для того, чтобы делать из него публичные выступления. Посмотрите, расскажите, интересно вообще?

Код с историей всего рефакторинга лежит на gist (19 ревизий).

Видео:

P.S.

Давненько я не говорил на аудиторию, даже заочно. Очень выматывает, и огрехов полно. Думаю, если убрать все задумчивые паузы, то можно сократить время в два раза.
Производство и обработка видео в Ubuntu сломаны. Совсем. Периодически слышу о том, что кто-то где-то пишет и развивает "профессиональные" видеоредакторы для Линукса, но при этом базовые юзкейсы или не работают вообще, или сопровождаются таким количеством багов, что возникает сомнение в том, что авторы программ вообще ими пользуются. Под базовыми юзкейсами я понимаю: переписать файл из фотоаппарата/видеокамеры в библиотеку, уменьшить размер файла с видео, обрезать концы, записать полноэкранный скринкаст.

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

Василий

Иван, а на каком железе у вас работает Ubuntu сейчас?

adw0rd

Вань, а чем конкретно пользовался для записи скринкаста? Я когда-то пользовался следующими продуктами в xubuntu:

Kazam Screencaster
RecordMyDesktop

Но с ними много проблем было, что в итоге я записал скринкаст на Windows, с другого компьютера.

Ivan Sagalaev

Я пользовался RecordMyDesktop в итоге, причём запускал его из терминала (в начале и в конце видео видно). Проблем у него полно, но с магической комбинацией случайно подобранных параметров сработало. Kazam не пробовал.

Ubuntu у меня работает на HP Envy 14 2011 года.

hare

Да, вот посмотришь, и понимаешь, насколько такие специализированные питон иде как Pycharm ушли вперед по сравнению с редакторами общего назначения с подсветкой. Там тебе и неиспользуемые переменные покажут, и переименования переменных по всему коду и PEP8-подсветка, не говоря уже о запуске программы хоткеями без прыжков в консоль и обратно.

Ivan Sagalaev

Согласен. Впрочем, я пользуюсь Sublime'ом очень примитивно, поэтому о нём самом судить по мне не надо. Насколько я знаю, он и PEP-8 умеет подсвечивать, и скрипты запускать. Просто я очень недавно с ним живу.

У редакторов общего назначения есть один плюс — то самое общее назначение. Если вы пишете не нескольких языках, не придётся под каждый искать специализированную IDE со своими правилами жизни.

hare

Это от языков зависит, например, я очень многие из них охватывает Intellij Idea с языковыми плагинами. Хотя я на экзотике не пишу, но мне в ней разве что C++ не хватало. Да, и запоздало благодарю за скринкаст :)

Ilya Kutukov

Даже в малых размерах интересно смотреть как работают другие кодеры. Хорошее решение с отказом от классов, которые, собственно, и не нужны.

За собой заметил забавную вещь, в свое время я приучал себя не бояться классов, наверное пик этого пришелся на изучение Java, потом наступила другая крайность, когда в коде начали плодиться классы одного метода, интерфейсные классы двух аттрибутов, геттеры, сеттеры и прочее. В итоге пришлось вникать в SICP и на новом уровне приучать себя не бояться функций.

ashamrin.livejournal.com

Я досмотрел до конца! Сам себе удивился :) Да, можно бесконечно смотреть, как горит огонь, течет вода и работает другой человек. По содержанию понравилось. Много поучительного: сделанный на коленке тест, diff html'я при откладке, и что-то невыразимое в словах :)

Георгий

Иван, спасибо за скринкаст. Мне, как начинающему, было очень интересно и познавательно. Надеюсь, что увижу еще не один рефакторинг.

hare, Вы попробуйте Sublime затюнить для работы с Python/Django. Я был в восторге от PyCharm'а, но через некоторое время все-таки перешел на Sublime: скорость работы этого редактора даже с десятком плагинов существенно выше скорости работы IDE, что и стало основной причиной. Ну и полнофункциональный Emmet - как его можно не любить?

Ivan Sagalaev

ashamrin:

diff html'я при откладке

А как иначе файлы сравнивать? Не глазами же…

и что-то невыразимое в словах :)

Я совсем не понял, о чём это, но рад, что тебе что-то было полезно :-)

Георгий:

Ну и полнофункциональный Emmet - как его можно не любить?

А что это?

Ilya Kutukov

скорость работы этого редактора даже с десятком плагинов существенно выше скорости работы IDE, что и стало основной причиной.

SSD, Memory cache и обрезка явно лишних интропекций неплохо помогают.

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

idlesign.ya.ru

[...] скорость работы этого редактора даже с десятком плагинов существенно выше скорости работы IDE, что и стало основной причиной.

Молодой человек, куда вы спешите? По Дерибасовской гуляют постепенно.

ruguevara

Жанр скринкаст-рефакторинга — это как детектив: ты уже раньше повестования догадался, где сидит баг и ЧСВ от этого увеличивается.

Георгий

Иван, Emmet (он же бывший Zen Coding) - весьма полезный тулкит для Web-разработки. Пример из официальной документации:

#page>div.logo+ul#navigation>li*5>a{Item $}

Превратится в

<div id="page">
  <div class="logo"></div>
  <ul id="navigation">
    <li><a href="">Item 1</a></li>
    <li><a href="">Item 2</a></li>
    <li><a href="">Item 3</a></li>
    <li><a href="">Item 4</a></li>
    <li><a href="">Item 5</a></li>
  </ul>
</div>

http://docs.emmet.io/

Молодой человек, куда вы спешите? По Дерибасовской гуляют постепенно.

Возможно по молодости лет.

Александр Артёменко

Ваня, в следующий раз редактор лучше переключить в fullscreen, и сделать шрифт покрупнее.

Miroff

Отлично.

Я бы еще захардкоженный путь тут же заменил на

sys.argv[1] if len(sys.argv) > 1 else '.'

и константу WIDTH переименовал в THUBMNAIL_WIDTH.

hare

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

B7W

Особо ценный комментарий

Насчет комментариев - http://www.python.org/dev/peps/pep-0257/ Хотя мне тоже нравится разделять.

И конечно огромное спасибо за скринкаст. Очень поучительно получилось. У меня обычно не хватает выдержки так поступательно править одно за другим.

voidwarrior

Спасибо за скринкаст.

В Sublime есть удобная штука по Ctrl+D, которая позволяет быстрее делать замены. Встаешь на идентификатор, зажимаешь Ctrl+D и сразу везде его редактируешь.

И в итоге все-таки появилось одно отличие, теперь скрипт не фейлится из-за отсутствия '/home/maniac/Desktop/4554182' на моей системе. Да и вообще, хорошо было бы в конце этот путь заменить на '.' :)

maxp

Кстати, зачем возвращать анонимный тупль, если есть namedtuple()? В нем ведь бесклассовая красота и сосредоточена :)

Ну и тоже вопрос - почему при выборе списка файлов list comprehension, а не генератор? Ведь laziness в таких случаях очень кстати.

maxp

Кстати, а почему незаслуженно забыт такой "питонячий" способ генерения html'я при помощи классов-тэгов? http://werkzeug.pocoo.org/docs/utils/#module-werkzeug.utils

maxp

Длинный os.path.basename можно иозбразить, как from os.path import basename as bname теоретически это даже будет быстрее работать, на бесконечно малую величину :)

piranha

Длинный os.path.basename можно иозбразить

Хе, я всегда делаю "import os.path as op", иначе эти длинные пути быстро начинают раздражать.

Artem Godlevskyy

Спасибо! Полезно и познавательно. Если найдутется время, запишите что-нибудь еще.

Георгий

hare, да, конечно. Для себя незначительно изменил такой набор:

https://github.com/jungo-git/sublime-text-2-configs

Ivan Sagalaev

Ваня, в следующий раз редактор лучше переключить в fullscreen, и сделать шрифт покрупнее.

Я тогда замучаюсь alt+tab'иться. Но вообще мне кажется, что если само видео на полный экран открыть, то всё достаточно крупно.

В Sublime есть удобная штука по Ctrl+D, которая позволяет быстрее делать замены. Встаешь на идентификатор, зажимаешь Ctrl+D и сразу везде его редактируешь.

Спасибо! Я верил, что должен быть какой-то шорткат на это. А есть возможность пропустить какое-то вхождение? Потому что иначе это мало отличается от просто поиска с заменой во фрагменте.

Кстати, зачем возвращать анонимный тупль, если есть namedtuple()?

Потому что namedtuple — порождение Диавола, и его никогда нельзя трогать!!!

Если серьёзно, то namedtuple — это, на мой взгляд, странный гибрид. Если нужны имена полей, есть dict. Если нужна ad-hoc структура из пары-тройки полей, то обычный tuple вполне справляется.

Длинный os.path.basename можно иозбразить, как from os.path import basename as bname

Переименование стандартных модулей и функций делает их менее узнаваемыми, а значит код с ними — хуже понимаемым. То есть, если человек видит в коде bname(filename), ему придёт идти искать, что это такое. А os.path.basename узнаётся сразу.

Антон

С diff было интересное решение. Я сам правда для таких целей использую file diff плагин для sublime. Может еще имеет смысл сделать html_group() вложенной функцией для функции html(), так как она нигде кроме этой самой функции html() использована быть не может.

def html(groups):
    """
    Generates HTML from groups of image duplicates
    """

    def html_group(group):
        return ''.join(
            '<img src="%s" width="%s"/>%s' % (os.path.basename(f), WIDTH, dist)
            for dist, f in group
        )

    body = '<hr/>'.join(html_group(g) for g in groups)
    return '<html><body>%s<hr/></body></html>' % body

Игорь

Спасибо большое, было очень интересно=) Побольше бы таких скринкастов.

sainaen

Отличный скринкаст! Сначала было испугался длины, но смотреть было интересно и, надеюсь, полезно, так что потраченное время «потерянным» назвать никак нельзя. Спасибо!

А есть возможность пропустить какое-то вхождение?

Да, конечно: ctrl+k, ctrl+d (последовательно) пропускает вхождение, которое выбралось последним ctrl+d.

q210

Все в скринкасте понравилось, за исключением одного момента — однобуквенные имена переменных это ужасно.

Пусть в простых list comprehensions это и не так бросается в глаза: "for f in files", "for g in groups" — c этим можно смириться. Однако как только вижу в коде даже unpacking в 2 переменные — к примеру у вас "from f, d in images" — сразу возникают непонятки... Кто такие f и d, какое отношение они имеют к списку с вроде бы картинками ? Приходится переводить взгляд на место, где этот список images создается, искать из чего состоят его элементы.

В то же время достаточно просто переименовать переменные в fname и image___data и их содержание становится понятно, ничто не мешает читать код дальше, не отвлекаясь на прыжки вверх-вниз в поисках расшивровки таинственных знаков.

zloyrusskiy

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

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

Односимвольные переменные - однозначно плохо (тем более там где надо читать справа-налево). Фаулер бы страдал от такого кода - 100% =)

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

Скрипт такого рода полезно иметь в виде дополнительного модуля, потому, наверное и рефакторить надо было именно в этом направлении.

И ещё в скринкасте постоянно упоминается о благотворном влиянии уменьшения количества строк кода, так вот Фаулер в своих книгах часто пишет об обратном и даже наоборот призывает писать больше кода, но более понятного.

Рефакторинг ради рефакторинга - пустая трата времени.

Несмотря на это скринкаст интересный, спасибо!

Ivan Sagalaev

ООП код проще тестировать юнит тестами (и в последствии функционал расширять)

Это голословно. Я не встречал практического подтверждения этому убеждению. Оно и логике противоречит, потому что объектная ориентация — синтаксический сахар для ограниченного набора юзкейсов, где в предметной области явно просматривается иерархия, а вот функциональная композиция — универсальное средство абстракции и создания систем любой сложности.

Односимвольные переменные - однозначно плохо (тем более там где надо читать справа-налево). Фаулер бы страдал от такого кода - 100% =)

Не стоит говорить за Фаулера. К тому же, "100%" и "однозначно" выдают в вас изрядную долю максимализма.

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

[...]

Рефакторинг ради рефакторинга - пустая трата времени.

В конкретно этом скрипте нет никакого практического смысла, это учебный материал. Соответственно, и в рефакторинге не было практического смысла, это была демострация того, как это вообще делается. И как показали отзывы, это время мы потратили совсем не зря.

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

Андрей Иванов

Очень понравился рефакторинг. Впечатляет. Благодаря вам пришёл в голову универсальный метод преобразования любого класса в функцию. Имеем класс C++

class A {
  A() {bodyA;}
  ~A() {body~A;}
  F1(arg1) {bodyF1;}
  F2(arg2) {bodyF2;}
  F3(arg3) {bodyF3;}
  F4(arg4) {bodyF4;}
}

Получаем функцию:

A(FN,arg1,arg2,arg3,arg4) {
  bodyA;
  switch(FN) {
    case 1: bodyF1;
    case 2: bodyF2;
    case 3: bodyF3;
    case 4: bodyF4;
    default: ;
  }
  body~A;
}

Аналогично, можно сделать "чистую" функцию из любой, которая использует глобальные переменные. Пусть есть грязная функция:

int global = 7;
int F(int arg) {
  int sum = 0;
  ....
  // используется глобальная переменная
  sum = global + arg;
  ....
  return sum;
}

Очищенный вариант

int cleanF(int arg, int g)
  ....
  // неиспользуется глобальная переменная
  sum = g + arg;
  ....
  return sum;
}

Вызов очищенной:

int = cleanF(5,global);

Денис Зайцев

Приятно снова услышать твой голос :)

p.s. 01:30:20. Не смотрю, не смотрю J

Ivan Sagalaev

Приятно снова услышать твой голос :)

Не изменился? :-)

p.s. 01:30:20. Не смотрю, не смотрю J

Эммм… Да там вроде ничего такого нет, что скрывать надо?

Victor Beresnev

большое спасибо за скринкаст, единственное что смущает - насколько оправдано использование %s для форматирования вместо str.format? просто я периодически наступил пару раз на неудобства в поведении % (автоматическое разворачиваниие tuple'ов итп) и больше им стараюсь не пользоваться.

bo858585

Может пост не совсем по теме - так как затрагивает также и содержание (алгоритм), а не только внутреннюю структуру кода, но все же замечу дополнительно к тому, о чем написал в комментариях к изначальному исходному коду в https://gist.github.com/bobuk/4522091: для изображения, которое является непрерывной функцией, т.е. харакетристики (RGB-составляющие) соседних пикселов плавно меняются), приближение разности двух таких изображений (которая тоже непрерывна, потому что разность н. есть н. функция) дискретной функцией (набор единичек и последующий подсчет их суммы) приводит к потерям точности - проверил это на нескольких выборках фоток. Поменял функцию рассчета разности матриц - исходную непрерывную функцию приближаю непрерывной же - так точность группировки лучше - https://gist.github.com/bo858585/5377492. Т.е. threshhold я убрал, а просто считаю разность матриц, затем для каждого элемента получившейся матрицы считаю метрику (корень из суммы квардратов трех чисел) и складываю эти метрики, сравнивая с суммой максимально возможных метрик (при максимально удаленных матрицах). Т.е. человеческий глаз похожесть изображений воспринимает и с учетом этого тоже - и алгоритм это должен учитывать.

Также при сравнении некоторых изображений, у которых с краев цвет один, имеет смысл обрезать края перед сравнением (а если изображение полностью однотонное, то оставлять 1 пиксел от него, например) - а то полностью белое полотно и белое полотно с неболшим черным текстом посередине будут отнесены к одной группе, а логичнее их разделить.

bo858585

Вот презентацию сделал по мотивам вышеизложенного - поиск похожих и копий картинки по заданной - https://docs.google.com/presentation/d/1WiVBMKCVTxg6IX_qmTtbHIcr3l5paTWeHktpRW72Y4c/edit?usp=sharing. Пока все локально - скорость работы поиска еще надо оптимизировать - занимаюсь в основном по праздникам и выходным - поэтому пока не успел дожать до нормальной скорости работы на большой базе изображений и сделать не(или слабо)зависимой скорость работы поиска от размера базы проиндексированных картинок. Как сделаю это - можно будет подумать над продакшн-версией. Договорились с ребятами с https://www.facebook.com/EvercodeTalks - что буду докладывать на следующей встрече об этом.

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