Comments 30
не используйте magic numbers, используйте константы
разделяйте отображение и внутреннее состояние игры
setAround дичь какая-то
ну и публикуйте полные исходники (хотя бы на github)
Что значит разделять отображение и внутренне состояние игры?
setAround себя оправдывает, без него приходилось повторять один и тот же момент кода 3-4 раза.
опубликуйте полные исходники, например на https://gist.github.com/, так будет проще комментировать происходящее, если конечно вам этот комментарий нужен :)
https://gist.github.com/dendefo/1b23e0f6d37d892c527cd9a686c93501
Что значит разделять отображение и внутренне состояние игры?
Это значит разделять логику, модель приложения от использования способа визуализации мира. Гугли MVC pattern. На Википедии, например, есть неплохая обзорная статья.
Зачем у тебя каждая клетка содержит информацию о координатах своих соседей? Эти сведения ведь можно получить моментально, зная координаты клетки и размеры игрового поля.
Morphostain
можете вынести в отдельный модуль (сделав простенький аналог settings в django) или же передавать как параметр в функции(можно даже объектом), главное чтобы это было явно указано.
Если вам надо таскать из функции в функцию одни и те же переменные, значит у Вас там на самом деле класс, скорее всего.
Кроме того, функции, неявно зависящие от глобального состояния, сложно тестировать.
Могу я попросить просмотреть код? Выше ссылка.
Обычно если есть такая цепочка, значит функция в середине цепочки (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
или откуда-то еще). Надеюсь, я понятно объясняю.
Если функция одна, то код выглядит она примерно так:
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). Сложный код структурно состоит из менее сложных компонентов, каждый из которых решает подзадачу основной задачи, и их по отдельности можно понять и протестировать. А в запутанном коде компоненты перемешаны и зависимости между ними проследить сложно. А значит понять и внести изменения в запутанный код без непредвиденных последствий трудно.
Также обратите внимание, как комментарии превратились в название функций. Если у участка кода комментарий более длинный, его можно положить в докстринг новой функции, то есть получится настоящая документация, которую через Sphinx опубликовать можно, например!
Это все, что я расказываю, важно в долгосрочной перспективе, потому что хороший код поддерживать проще. Если надо что-то сделать быстро на один раз, то можно и не заморачиваться :)
Возможно, я изначально неверную архитектуру взял. Я стараюсь все документировать, чтобы легче дорабатывать было.
У Вас image_process
это как будто main
подпроцессов. Не понятно, почему некоторые данные из main
родительского процесса передаются в image_process
напрямую через аргументы, а некоторые через глобальный args. Более того, некоторые вещи там продублированы. Например, ничто не мешает другому программисту (или даже не другому) случайно использовать args.thesh0
вместо thresh0
, что может привести к ошибкам (например когда thresh0 не был указан при вызове и его надо было добыть из json). Такую ошибку сделать сложнее, если есть только один источник входных данных.
В общем, входные параметры не перестают быть входными если их передать в функцию в обход списка ее аргументов. Просто в таком случае за ними следить надо бдительнее. Проще просто не использовать глобальные переменные.
Парсить каждый раз не надо. Ведь Вы уже реализовали все что нужно тут: 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, и передавать все (или сколько надо) за раз.
Globals совсем не вариант.
Вы написали свою первую версию программы. Теперь её перепишите так, чтобы она работала так же, но без globals.
И циклы for j in range(len(buttons[i]))
нужно менять на просто for button in buttons
.
Ваш код — хороший пример того, как написать можно, но не нужно. Его нужно эволюционировать до чего-то более совершенного. В комментариях исправлять не получится, нужно видеть историю изменений.
Лучше залейте его на гитхаб, чтобы можно было его улучшать итеративно.
Советую почитать книгу "Чистый код" Роберта Мартина. Там, правда, не питон, но основные советы годятся.
def game()можно было написать короче.
зачем вам методы setXXX и viewXXX? Почему не обращаться к полю непосредственно? В питоне же всё открыто, подобное сокрытие не имеет смысла.
И ещё, попробуйте отделить данные от представления. Пусть данные хранятся в массиве (списке) как целые числа. Откажитесь от класса Pole. Вам не нужно у каждой клетки иметь список соседних (это легко подсчитать и так). Как вариант — вообще отказаться от массива всех ячеек. Можно иметь лишь список с координатами бомб.
Да, и ещё. Всё-таки классы нужно называть с большой буквы.
Да, посмотрел. Только вы в гист сохранили, а это, как я понял, сервис снипетов. Я же советую в сам github залить, чтобы разные версии коммитить.
Почему не лучшая мысль?! Нормальная мысль.
Вы хотите узнать, как сделать этот код лучше?! Если да, то создавайте.
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
Создание сапера при помощи модуля Tkinter