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

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

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

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

Видео:

P.S.

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

  1. Василий

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

  2. adw0rd

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

    • Kazam Screencaster
    • RecordMyDesktop

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

  3. Ivan Sagalaev

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

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

  4. hare

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

  5. Ivan Sagalaev

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

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

  6. hare

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

  7. Ilya Kutukov

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

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

  8. ashamrin.livejournal.com

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

  9. Георгий

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

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

  10. Ivan Sagalaev

    ashamrin:

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

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

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

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

    Георгий:

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

    А что это?

  11. Ilya Kutukov

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

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

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

  12. idlesign.ya.ru

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

    -

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

  13. ruguevara

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

  14. Георгий

    Иван, 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/

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

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

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

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

  16. Miroff

    Отлично.

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

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

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

  17. hare

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

  18. B7W

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

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

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

  19. voidwarrior

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

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

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

  20. maxp

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

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

  21. maxp

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

  22. maxp

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

  23. piranha

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

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

  24. Artem Godlevskyy

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

  25. Георгий

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

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

  26. 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 узнаётся сразу.

  27. Антон

    С 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
    
  28. Игорь

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

  29. sainaen

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

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

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

  30. 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 и их содержание становится понятно, ничто не мешает читать код дальше, не отвлекаясь на прыжки вверх-вниз в поисках расшивровки таинственных знаков.

  31. zloyrusskiy

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

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

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

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

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

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

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

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

  32. Ivan Sagalaev

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

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

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

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

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

    [...]

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

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

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

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

    Очень понравился рефакторинг. Впечатляет. Благодаря вам пришёл в голову универсальный метод преобразования любого класса в функцию. Имеем класс 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);
    
  34. Денис Зайцев

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

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

  35. Ivan Sagalaev

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

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

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

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

  36. Victor Beresnev

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

  37. bo858585

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

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

  38. bo858585

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

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

Format with markdown