Pull to refresh

Comments 30

не используйте globals
не используйте magic numbers, используйте константы
разделяйте отображение и внутреннее состояние игры
setAround дичь какая-то
ну и публикуйте полные исходники (хотя бы на github)
Чем заменять global?
Что значит разделять отображение и внутренне состояние игры?
setAround себя оправдывает, без него приходилось повторять один и тот же момент кода 3-4 раза.
если по-другому спроектировать работу программы, то использовать global не потребуется

опубликуйте полные исходники, например на https://gist.github.com/, так будет проще комментировать происходящее, если конечно вам этот комментарий нужен :)

Ниже orgkhnargh предложил занести все это под класс. Думаю здесь это будет как раз кстати. Ну и код:
https://gist.github.com/dendefo/1b23e0f6d37d892c527cd9a686c93501
Что значит разделять отображение и внутренне состояние игры?

Это значит разделять логику, модель приложения от использования способа визуализации мира. Гугли MVC pattern. На Википедии, например, есть неплохая обзорная статья.

Зачем у тебя каждая клетка содержит информацию о координатах своих соседей? Эти сведения ведь можно получить моментально, зная координаты клетки и размеры игрового поля.
Globals совсем не вариант использовать? Я к ним пришел для передачи аргументов запуска программы функциям. Просто одной нужно знать args.silent, другой args.thresh0. Таскать их неудобно из функции в функцию. Плюс это константы по сути для каждого запуска.
Morphostain
я не говорю, что использовать globals нельзя, лишь то, что нужно понимать когда это оправданно, а когда нет, ну и самособой принимая риски использования.

можете вынести в отдельный модуль (сделав простенький аналог settings в django) или же передавать как параметр в функции(можно даже объектом), главное чтобы это было явно указано.

Если вам надо таскать из функции в функцию одни и те же переменные, значит у Вас там на самом деле класс, скорее всего.


Кроме того, функции, неявно зависящие от глобального состояния, сложно тестировать.

Я не профессионал. Глобальные у меня опции, ключи запуска. Они неизменны. Используются по всему коду в разных функциях. Где-то опция silent-режима, где-то сохранение дополнительных каналов изображений. Используются однократно фактически. Но, так как они парсятся вначале, то дальше придется передавать их по цепочке функций. Что-то вроде f1(args)-> f2(args)-> f3(args) а вот здесь мы наконец используем один из args.
Могу я попросить просмотреть код? Выше ссылка.

Обычно если есть такая цепочка, значит функция в середине цепочки (f2) это на самом деле не одна функция, а несколько.


Давайте рассмотрим упрощенный пример:


def process_args(arg1, arg2, arg3):
    """Точка входа в программу.
    arg1 - собственный аргумент. Остальные аргументы передаются дальше.
    """
    print(arg1)
    f2(arg2, arg3)

def f2(arg2, arg3):
    """Зависит только от arg2.
    arg3 тут только, чтобы передать в f3().
    """
    data = arg2
    data[0] = 42
    f3(data, arg3)
    print(data)

def f3(arg2, arg3):
    print(arg2, arg3)

Тут process_args некоторым образом обрабатывает некоторые данные. Обратите внимание, что f3 зависит от промежуточного состояния f2. Казалось бы, деваться некуда, придется либо arg3 делать глобальным, либо тянуть его по цепочке. На самом деле эту проблему можно легко решить, осознав, что f2 на самом деле производит 2 разных действия, и если их на самом деле разбить на 2 отдельные функции, то проблема с цепочкой исчезнет. Вот так:


def process_args(arg1, arg2, arg3):
    """Точка входа в программу.
    arg1 - собственный аргумент. Остальные аргументы передаются дальше.
    """
    print(arg1)
    intermediate_f2_result = f2a(arg2)
    f2b(intermediate_f2_result, arg3)

def f2a(arg2):
    """Зависит только от arg2."""
    data = arg2
    data[0] = 42
    return data

def f2b(arg2_processed, arg3):
    """Зависит и от arg2_processed и от arg3.
    Более того, зависимость от обоих аргументов указана явно!
    """
    f3(arg2_processed, arg3)
    print(arg2_processed)

def f3(arg2, arg3):
    print(arg2, arg3)

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


def process_args(arg1, arg2, arg3):
    """Точка входа в программу.
    arg1 - собственный аргумент. Остальные аргументы передаются дальше.
    """
    print(arg1)

    def callback(arg2_processed):
        # Ссылку на arg3 эта функция получит из области видимости process_args,
        # так что протаскивать arg3 в f2 не нужно.
        f3(arg2_processed, arg3)

    f2(arg2, callback)  # тут arg3 больше не фигурирует

def f2(arg2, cb):
    """Зависит только от arg2."""
    data = arg2
    data[0] = 42
    cb(data)  # вместо f3; arg3 тут не передаем, но f3 его все равно получит
    print(data)

def f3(arg2, arg3):
    print(arg2, arg3)

У Вас в Morphostain есть эта проблема в group_analyze, например. Если ее раздробить на отдельные функции, то не будет необходимости использовать глобальные args в plot_group. На самом деле определить, нуждается ли функция в разделении можно, подобрав ей правильное название. Я бы group_analyze назвал group_analyze_and_print_and_plot. Видите, уже 3 функции напрашивается.


Если организовать код немного по-другому, необходимость тянуть аргументы из функции в функцию, используя их только на самом низком уровне, отпадает. А если функция действительно использует параметр, то нет причин получать его откуда-то кроме списка аргументов (кроме самого высокого уровня, функции main, где списка аргументов нет, и входные данные надо явно доставать из sys.argv или откуда-то еще). Надеюсь, я понятно объясняю.

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

Если функция одна, то код выглядит она примерно так:


def f():
    # do one thing
    x = 87
    y = x * 3

    # do another thing
    z = y / 7
    return z

А если несколько, то так:


def f():
    y = do_one_thing()
    z = do_another_thing(y)
    return z

def do_one_thing():
    x = 87
    return x * 3

def do_another_thing(y):
    z = y / 7
    return z

Ухудшает ли это читаемость, или улучшает — вопрос субъективный. Я лично считаю, что улучшает, особенно читаемость функции f. Так становится проще определить что функция делает, а не как.


Фишка в том, что если задача сама по себе сложная (complex), то код ее решения будет как минимум таким же сложным (complex), иначе он будет запутнным (complicated). Сложный код структурно состоит из менее сложных компонентов, каждый из которых решает подзадачу основной задачи, и их по отдельности можно понять и протестировать. А в запутанном коде компоненты перемешаны и зависимости между ними проследить сложно. А значит понять и внести изменения в запутанный код без непредвиденных последствий трудно.

С этим согласен. Сложно с нуля все это изучать) В моем варианте это типичный WTF driven development.

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


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

Я стараюсь сразу придерживаться хороших практик. Даже не смотря на малую вероятность дальнейшего развития глобального. Архитектуру очень поломало внезапное появление асинхронности с использованием​ pool.
Хотя там непросто будет завязать все на Main. Я с многопроцессностью довольно успешно игрался там. Запутанно может получиться. Сейчас логика выглядит так: main делает самые основные вызовы и отдает серию картинок в pool. Там запускается тот кусок, который нужно параллельно исполнять — pipeline для обработки и получения данных от одной картинки из серии. Дергается куча функций, которые это изображение поэтапно превращают в данные. Pool завершает свою работу и отдает пачку данных об обработанных изображениях. Main эти данные уже немного причесывает, при необходимости анализирует статистику и подает на сохранение.

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

У Вас image_process это как будто main подпроцессов. Не понятно, почему некоторые данные из main родительского процесса передаются в image_process напрямую через аргументы, а некоторые через глобальный args. Более того, некоторые вещи там продублированы. Например, ничто не мешает другому программисту (или даже не другому) случайно использовать args.thesh0 вместо thresh0, что может привести к ошибкам (например когда thresh0 не был указан при вызове и его надо было добыть из json). Такую ошибку сделать сложнее, если есть только один источник входных данных.


В общем, входные параметры не перестают быть входными если их передать в функцию в обход списка ее аргументов. Просто в таком случае за ними следить надо бдительнее. Проще просто не использовать глобальные переменные.

Смотри, args надо распарсить один раз, до разделения на процессы. Парсить его каждый раз в image_process странно, хотя и не накладно по ресурсам. Но неэлегантно, с учетом того, что args.matrix задействует обращение к внешнему json и чтению из файла. Как передать это все внутренним функциям image_process, с учетом ее дробления на независимые процессы?

Парсить каждый раз не надо. Ведь Вы уже реализовали все что нужно тут: https://github.com/meklon/morphostain/blob/660102bc1b11641e845966754b4dff80cca905fc/morphostain/morphostain.py#L460-L461


Почему var_pause, matrix_stains, path_output, path_output_log, str_ch0, str_ch1, str_ch2, thresh_0 и thresh_1 там передаются, а dpi нет? Ведь все эти значения тоже "константы по сути для каждого запуска".


Если кажется, что список аргументов слишком длинный, но все аргументы действительно нужны, то можно их собрать в словарь, объект или namedtuple, и передавать все (или сколько надо) за раз.

Да, логично. Просто этот wrapper сам по себе страшно выглядит. Жуткое количество аргументов, некрасиво для читаемости. Но согласен в целом. Просто у меня по факту две крупные функции. Main() и image_process(), которая конвейер обработки конкретного изображения описывает. Спасибо. Буду думать над рефакторингом. Я еще и на проверку входящих данных забил, да. Утилита для себя, понятно, если падает от подачи некорректных файлов или [] на вход.

Globals совсем не вариант.
Вы написали свою первую версию программы. Теперь её перепишите так, чтобы она работала так же, но без globals.


И циклы for j in range(len(buttons[i])) нужно менять на просто for button in buttons.


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


Советую почитать книгу "Чистый код" Роберта Мартина. Там, правда, не питон, но основные советы годятся.

Изначально некоторые циклы так и выглядели, но от этого пришлось отказаться, потому что мне нужен был индекс элемента в массиве, а не сам элемент. Хотя да, в одном моменте в
def game()
можно было написать короче.

зачем вам методы setXXX и viewXXX? Почему не обращаться к полю непосредственно? В питоне же всё открыто, подобное сокрытие не имеет смысла.


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


Да, и ещё. Всё-таки классы нужно называть с большой буквы.

Around как раз и подсчитывает, а делать это несколько раз в разных местах — глупо. Насчет циклов и set/view исправился. Ссылка на гит выше.

Да, посмотрел. Только вы в гист сохранили, а это, как я понял, сервис снипетов. Я же советую в сам github залить, чтобы разные версии коммитить.

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

Почему не лучшая мысль?! Нормальная мысль.
Вы хотите узнать, как сделать этот код лучше?! Если да, то создавайте.

Звучит так, будто вы заманиваете меня на темную сторону силы. https://github.com/dendefo/MineSweeper А там есть печеньки?

Знания — однозначно сила. А вот в какой цвет вы эту силу окрасите, зависит от вас.
Завтра попробую пул реквесты поотправлять с предложениями об улучшении кода.
Думаю, мне удастся показать, как избавиться от globals и сделать код стройным и понятным не только интерпретатору.

для этого есть enumerate:
In [1]: some_list = [6,7,8,9]

In [2]: for i, item in enumerate(some_list):
   ...:     print('i={} item={}'.format(i, item))

i=0 item=6
i=1 item=7
i=2 item=8
i=3 item=9
Sign up to leave a comment.

Articles