День добрый. Почти каждый начинающий программист стремится к созданию своей первой игры. Спустя пол года ленивого кропотливого обучения я решился написать сапера. Языком написания был выбран Python, модулем для добавления интерфейса tkinter, потому как уже имелся опыт работы с ним. Этот пост будет полезен скорее начинающим кодерам, но если вы итак все знаете, можете написать свои советы по улучшению кода в комменты.
Приступим. Первым делом нужно было определиться, что будет собой представлять клетка. Самое выгодное решение — создать класс поля:
Теперь надо создать интерфейс для настройки игры:
В итоге получаем вот такое вот окно:
![image](https://habrastorage.org/files/cc2/732/f26/cc2732f26c87430c9f63373b74049c66.png)
Теперь нужно прописать функцию bombcounter
Теперь приступаем к основной части, написанию функции игры:
У нас появилось целых три функции, которые нужно написать. Начнем с .setAround():
Все, что здесь происходит, это заполнение массива self.around. Мы рассматриваем различные случаи и на выходе получаем верный ответ. Если есть варианты, как сделать это проще, я приму их во внимание.
Пишем view()
Итак. Сейчас у нас написаны функции: открытия клетки, заполнения массива around, начала игры и получения значения насчет размера игрового поля и кол-ва мин. Но до сих пор нет функции для установки мин. Исправляемся:
И вторая важная для нас функция: setValue()
На этом заканчивается основная часть. Игра может работать прямо сейчас, но без установки флажка и определения победы/проигрыша. Тут все просто. Установка флажка:
Функции lose() и winer() просты и не требуют объяснений. Если будет нужно, напишу в комменты.
Финальный вид:
![image](https://habrastorage.org/files/973/36a/7f7/97336a7f79854963afae096e03bd106e.png)
Свои вопросы, предложения и критику пишите в комменты, постараюсь ответить, обсудить и подумать.
Приступим. Первым делом нужно было определиться, что будет собой представлять клетка. Самое выгодное решение — создать класс поля:
class pole(object): #создаем Класс поля, наследуемся от Object
def __init__(self,master,row, column): #Инициализация поля. master - окно Tk().
self.button = Button(master, text = ' ') #Создаем для нашего поля атрибут 'button'
self.mine = False #Переменная наличия мины в поле
self.value = 0 #Кол-во мин вокруг
self.viewed = False #Открыто/закрыто поле
self.flag = 0 #0 - флага нет, 1 - флаг стоит, 2 - стоит "?"
self.around = [] #Массив, содержащий координаты соседних клеток
self.clr = 'black' #Цвет текста
self.bg = None #Цвет фона
self.row = row #Строка
self.column = column #Столбец
Теперь надо создать интерфейс для настройки игры:
settings = Tk() #Создаем окно
settings.title('Настройки') #Пишем название окна
settings.geometry('200x150') #Задаем размер
mineText = Text(settings, width = 5, height = 1) #Создаем поля для ввода текста и пояснения
mineLabe = Label(settings, height = 1, text = 'Бомбы:')
highText = Text(settings, width = 5, height = 1)
highLabe = Label(settings, height = 1, text = 'Ширина:')
lenghtText = Text(settings, width = 5, height = 1)
lenghtLabe = Label(settings, height = 1, text = 'Высота:')
mineBut = Button(settings, text = 'Начать:', command = bombcounter) #Создаем кнопку
mineBut.place(x = 70, y = 90) #Размещаем это все
mineText.place(x = 75, y = 5)
mineLabe.place(x = 5, y = 5)
highText.place(x = 75, y = 30)
highLabe.place(x = 5, y = 30)
lenghtText.place(x = 75, y = 55)
lenghtLabe.place(x = 5, y = 55)
settings.mainloop()
В итоге получаем вот такое вот окно:
![image](https://habrastorage.org/files/cc2/732/f26/cc2732f26c87430c9f63373b74049c66.png)
Теперь нужно прописать функцию bombcounter
def bombcounter():
global bombs
if mineText.get('1.0', END) == '\n': #Проверяем наличие текста
bombs = 10 #Если текста нет, то по стандарту кол-во бомб - 10
else:
bombs = int(mineText.get('1.0', END)) #Если текст есть, то это и будет кол-во бомб
if highText.get('1.0', END) == '\n':
high = 9
else:
high = int(highText.get('1.0', END))
if lenghtText.get('1.0', END) == '\n':
lenght = 9
else:
lenght = int(lenghtText.get('1.0', END))
game(high,lenght) #Начинаем игру, передавая кол-во полей
Теперь приступаем к основной части, написанию функции игры:
def game(high,lenght): #получаем значения
root = Tk()
root.title('Сапер')
global buttons
global mines
global flags
flags = [] #Массив, содержащий в себе места, где стоят флажки
mines = [] #Массив, содержащий в себе места, где лежат мины
buttons = [[pole(root,j,i) for i in range(high)] for j in range(lenght)] #Двумерный массив, в котором лежат поля
for i in range(len(buttons)): #Цикл по строкам
for j in range(len(buttons[i])): #Цикл по элементам строки
buttons[i][j].button.grid(column = j, row = i, ipadx = 7, ipady = 1) #Размещаем все в одной сетке при помощи grid
buttons[i][j].button.bind('<Button-1>', buttons[i][j].view) #Биндим открывание клетки
buttons[i][j].button.bind('<Button-3>', buttons[i][j].setFlag) #Установка флажка
buttons[i][j].setAround() #Функция заполнения массива self.around
buttons[0][0].button.bind('<Control-Button-1>', cheat) #создаем комбинацию клавиш для быстрого решения
root.resizable(False,False) #запрещаем изменения размера
root.mainloop()
У нас появилось целых три функции, которые нужно написать. Начнем с .setAround():
def setAround(self):
if self.row == 0:
self.around.append([self.row+1,self.column])
if self.column == 0:
self.around.append([self.row,self.column+1])
self.around.append([self.row+1,self.column+1])
elif self.column == len(buttons[self.row])-1:
self.around.append([self.row,self.column-1])
self.around.append([self.row+1,self.column-1])
else:
self.around.append([self.row,self.column-1])
self.around.append([self.row,self.column+1])
self.around.append([self.row+1,self.column+1])
self.around.append([self.row+1,self.column-1])
elif self.row == len(buttons)-1:
self.around.append([self.row-1,self.column])
if self.column == 0:
self.around.append([self.row,self.column+1])
self.around.append([self.row-1,self.column+1])
elif self.column == len(buttons[self.row])-1:
self.around.append([self.row,self.column-1])
self.around.append([self.row-1,self.column-1])
else:
self.around.append([self.row,self.column-1])
self.around.append([self.row,self.column+1])
self.around.append([self.row-1,self.column+1])
self.around.append([self.row-1,self.column-1])
else:
self.around.append([self.row-1,self.column])
self.around.append([self.row+1,self.column])
if self.column == 0:
self.around.append([self.row,self.column+1])
self.around.append([self.row+1,self.column+1])
self.around.append([self.row-1,self.column+1])
elif self.column == len(buttons[self.row])-1:
self.around.append([self.row,self.column-1])
self.around.append([self.row+1,self.column-1])
self.around.append([self.row-1,self.column-1])
else:
self.around.append([self.row,self.column-1])
self.around.append([self.row,self.column+1])
self.around.append([self.row+1,self.column+1])
self.around.append([self.row+1,self.column-1])
self.around.append([self.row-1,self.column+1])
self.around.append([self.row-1,self.column-1])
Все, что здесь происходит, это заполнение массива self.around. Мы рассматриваем различные случаи и на выходе получаем верный ответ. Если есть варианты, как сделать это проще, я приму их во внимание.
Пишем view()
def view(self,event):
if mines == []: #При первом нажатии
seter(0,self.around,self.row,self.column) #Устанавливаем мины
if self.value == 0: #Устанавливаем цвета. Можно написать и для 6,7 и 8, но у меня закончилась фантазия
self.clr = 'yellow'
self.value = None
self.bg = 'lightgrey'
elif self.value == 1:
self.clr = 'green'
elif self.value == 2:
self.clr = 'blue'
elif self.value == 3:
self.clr = 'red'
elif self.value == 4:
self.clr = 'purple'
if self.mine and not self.viewed and not self.flag: #Если в клетке есть мина, она еще не открыта и на ней нет флага
self.button.configure(text = 'B', bg = 'red') #Показываем пользователю, что тут есть мина
self.viewed = True #Говорим, что клетка раскрыта
for q in mines:
buttons[q[0]][q[1]].view('<Button-1>') #Я сейчас буду вскрывать ВСЕ мины
lose() #Вызываем окно проигрыша
elif not self.viewed and not self.flag: #Если мины нет, клетка не открыта и флаг не стоит
self.button.configure(text = self.value, fg = self.clr, bg = self.bg) #выводим в текст поля значение
self.viewed = True
if self.value == None: #Если вокруг нет мин
for k in self.around:
buttons[k[0]][k[1]].view('<Button-1>') #Открываем все поля вокруг
Итак. Сейчас у нас написаны функции: открытия клетки, заполнения массива around, начала игры и получения значения насчет размера игрового поля и кол-ва мин. Но до сих пор нет функции для установки мин. Исправляемся:
def seter(q, around,row,column): #Получаем массив полей вокруг и координаты нажатого поля
if q == bombs: #Если кол-во установленных бомб = кол-ву заявленных
for i in range(len(buttons)): #Шагаем по строкам
for j in range(len(buttons[i])): #Шагаем по полям в строке i
for k in buttons[i][j].viewAround(): #Шагаем по полям вокруг выбранного поля j
if buttons[k[0]][k[1]].viewMine(): #Если в одном из полей k мина
buttons[i][j].setValue(buttons[i][j].viewValue()+1) #То увеличиваем значение поля j
return
a = choice(buttons) #Выбираем рандомную строку
b = choice(a) #Рандомное поле
if [buttons.index(a),a.index(b)] not in mines and [buttons.index(a),a.index(b)] not in around and [buttons.index(a),a.index(b)] != [row,column]: #Проверяем, что выбранное поле не выбиралось до этого и, что не является полем на которую мы нажали (или окружающим ее полем)
b.setMine() #Ставим мину
mines.append([buttons.index(a),a.index(b)]) #Добавляем ее в массив
seter(q+1,around,row,column) #Вызываем установщик, сказав, что одна мина уже есть
else:
seter(q,around,row,column) #Вызываем установщик еще раз
И вторая важная для нас функция: setValue()
def setValue(self,value):
self.value = value
На этом заканчивается основная часть. Игра может работать прямо сейчас, но без установки флажка и определения победы/проигрыша. Тут все просто. Установка флажка:
def setFlag(self,event):
if self.flag == 0 and not self.viewed: #Если поле не открыто и флага нет
self.flag = 1 #Ставим флаг
self.button.configure(text = 'F', bg = 'yellow') #Выводим флаг
flags.append([self.row,self.column]) #Добавляем в массив флагов
elif self.flag == 1: #Если флаг стоим
self.flag = 2 #Ставим значение '?'
self.button.configure(text = '?', bg = 'blue') #Выводим его
flags.pop(flags.index([self.row,self.column])) #Удаляем флаг из массива флагов
elif self.flag == 2: #Если вопрос
self.flag = 0 #Устанавливаем на отсутствие флага
self.button.configure(text = ' ', bg = 'white') #Выводим пустоту
if sorted(mines) == sorted(flags) and mines != []: #если массив флагов идентичен массиву мин
winer() #Сообщаем о победе
Функции lose() и winer() просты и не требуют объяснений. Если будет нужно, напишу в комменты.
Финальный вид:
![image](https://habrastorage.org/files/973/36a/7f7/97336a7f79854963afae096e03bd106e.png)
Свои вопросы, предложения и критику пишите в комменты, постараюсь ответить, обсудить и подумать.
Поделиться с друзьями
ahmpro
не используйте globals
не используйте magic numbers, используйте константы
разделяйте отображение и внутреннее состояние игры
setAround дичь какая-то
ну и публикуйте полные исходники (хотя бы на github)
Dendefo
Чем заменять global?
Что значит разделять отображение и внутренне состояние игры?
setAround себя оправдывает, без него приходилось повторять один и тот же момент кода 3-4 раза.
ahmpro
если по-другому спроектировать работу программы, то использовать global не потребуется
опубликуйте полные исходники, например на https://gist.github.com/, так будет проще комментировать происходящее, если конечно вам этот комментарий нужен :)
Dendefo
Ниже orgkhnargh предложил занести все это под класс. Думаю здесь это будет как раз кстати. Ну и код:
https://gist.github.com/dendefo/1b23e0f6d37d892c527cd9a686c93501
GoldJee
Это значит разделять логику, модель приложения от использования способа визуализации мира. Гугли MVC pattern. На Википедии, например, есть неплохая обзорная статья.
Зачем у тебя каждая клетка содержит информацию о координатах своих соседей? Эти сведения ведь можно получить моментально, зная координаты клетки и размеры игрового поля.
Meklon
Globals совсем не вариант использовать? Я к ним пришел для передачи аргументов запуска программы функциям. Просто одной нужно знать args.silent, другой args.thresh0. Таскать их неудобно из функции в функцию. Плюс это константы по сути для каждого запуска.
Morphostain
ahmpro
я не говорю, что использовать globals нельзя, лишь то, что нужно понимать когда это оправданно, а когда нет, ну и самособой принимая риски использования.
можете вынести в отдельный модуль (сделав простенький аналог settings в django) или же передавать как параметр в функции(можно даже объектом), главное чтобы это было явно указано.
orgkhnargh
Если вам надо таскать из функции в функцию одни и те же переменные, значит у Вас там на самом деле класс, скорее всего.
Кроме того, функции, неявно зависящие от глобального состояния, сложно тестировать.
Meklon
Я не профессионал. Глобальные у меня опции, ключи запуска. Они неизменны. Используются по всему коду в разных функциях. Где-то опция silent-режима, где-то сохранение дополнительных каналов изображений. Используются однократно фактически. Но, так как они парсятся вначале, то дальше придется передавать их по цепочке функций. Что-то вроде f1(args)-> f2(args)-> f3(args) а вот здесь мы наконец используем один из args.
Могу я попросить просмотреть код? Выше ссылка.
orgkhnargh
Обычно если есть такая цепочка, значит функция в середине цепочки (
f2
) это на самом деле не одна функция, а несколько.Давайте рассмотрим упрощенный пример:
Тут process_args некоторым образом обрабатывает некоторые данные. Обратите внимание, что
f3
зависит от промежуточного состоянияf2
. Казалось бы, деваться некуда, придется либоarg3
делать глобальным, либо тянуть его по цепочке. На самом деле эту проблему можно легко решить, осознав, чтоf2
на самом деле производит 2 разных действия, и если их на самом деле разбить на 2 отдельные функции, то проблема с цепочкой исчезнет. Вот так:Аналогичного результата можно добиться, используя функцию обратного вызова:
У Вас в Morphostain есть эта проблема в
group_analyze
, например. Если ее раздробить на отдельные функции, то не будет необходимости использовать глобальные args вplot_group
. На самом деле определить, нуждается ли функция в разделении можно, подобрав ей правильное название. Я быgroup_analyze
назвалgroup_analyze_and_print_and_plot
. Видите, уже 3 функции напрашивается.Если организовать код немного по-другому, необходимость тянуть аргументы из функции в функцию, используя их только на самом низком уровне, отпадает. А если функция действительно использует параметр, то нет причин получать его откуда-то кроме списка аргументов (кроме самого высокого уровня, функции
main
, где списка аргументов нет, и входные данные надо явно доставать изsys.argv
или откуда-то еще). Надеюсь, я понятно объясняю.Meklon
О. Отлично, спасибо. Я подумаю. Просто у меня всегда возникал вопрос — где разумный предел дробления функций. Много микрофункций по 3 строчки тоже читаемость ухудшают, как и огромные куски.
orgkhnargh
Если функция одна, то код выглядит она примерно так:
А если несколько, то так:
Ухудшает ли это читаемость, или улучшает — вопрос субъективный. Я лично считаю, что улучшает, особенно читаемость функции
f
. Так становится проще определить что функция делает, а не как.Фишка в том, что если задача сама по себе сложная (complex), то код ее решения будет как минимум таким же сложным (complex), иначе он будет запутнным (complicated). Сложный код структурно состоит из менее сложных компонентов, каждый из которых решает подзадачу основной задачи, и их по отдельности можно понять и протестировать. А в запутанном коде компоненты перемешаны и зависимости между ними проследить сложно. А значит понять и внести изменения в запутанный код без непредвиденных последствий трудно.
Meklon
С этим согласен. Сложно с нуля все это изучать) В моем варианте это типичный WTF driven development.
orgkhnargh
Также обратите внимание, как комментарии превратились в название функций. Если у участка кода комментарий более длинный, его можно положить в докстринг новой функции, то есть получится настоящая документация, которую через Sphinx опубликовать можно, например!
Это все, что я расказываю, важно в долгосрочной перспективе, потому что хороший код поддерживать проще. Если надо что-то сделать быстро на один раз, то можно и не заморачиваться :)
Meklon
Я стараюсь сразу придерживаться хороших практик. Даже не смотря на малую вероятность дальнейшего развития глобального. Архитектуру очень поломало внезапное появление асинхронности с использованием? pool.
Meklon
Хотя там непросто будет завязать все на Main. Я с многопроцессностью довольно успешно игрался там. Запутанно может получиться. Сейчас логика выглядит так: main делает самые основные вызовы и отдает серию картинок в pool. Там запускается тот кусок, который нужно параллельно исполнять — pipeline для обработки и получения данных от одной картинки из серии. Дергается куча функций, которые это изображение поэтапно превращают в данные. Pool завершает свою работу и отдает пачку данных об обработанных изображениях. Main эти данные уже немного причесывает, при необходимости анализирует статистику и подает на сохранение.
Возможно, я изначально неверную архитектуру взял. Я стараюсь все документировать, чтобы легче дорабатывать было.
orgkhnargh
У Вас
image_process
это как будтоmain
подпроцессов. Не понятно, почему некоторые данные изmain
родительского процесса передаются вimage_process
напрямую через аргументы, а некоторые через глобальный args. Более того, некоторые вещи там продублированы. Например, ничто не мешает другому программисту (или даже не другому) случайно использоватьargs.thesh0
вместоthresh0
, что может привести к ошибкам (например когда thresh0 не был указан при вызове и его надо было добыть из json). Такую ошибку сделать сложнее, если есть только один источник входных данных.В общем, входные параметры не перестают быть входными если их передать в функцию в обход списка ее аргументов. Просто в таком случае за ними следить надо бдительнее. Проще просто не использовать глобальные переменные.
Meklon
Смотри, args надо распарсить один раз, до разделения на процессы. Парсить его каждый раз в image_process странно, хотя и не накладно по ресурсам. Но неэлегантно, с учетом того, что args.matrix задействует обращение к внешнему json и чтению из файла. Как передать это все внутренним функциям image_process, с учетом ее дробления на независимые процессы?
orgkhnargh
Парсить каждый раз не надо. Ведь Вы уже реализовали все что нужно тут: 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, и передавать все (или сколько надо) за раз.
Meklon
Да, логично. Просто этот wrapper сам по себе страшно выглядит. Жуткое количество аргументов, некрасиво для читаемости. Но согласен в целом. Просто у меня по факту две крупные функции. Main() и image_process(), которая конвейер обработки конкретного изображения описывает. Спасибо. Буду думать над рефакторингом. Я еще и на проверку входящих данных забил, да. Утилита для себя, понятно, если падает от подачи некорректных файлов или [] на вход.
CJay
Globals совсем не вариант.
Вы написали свою первую версию программы. Теперь её перепишите так, чтобы она работала так же, но без globals.
И циклы
for j in range(len(buttons[i]))
нужно менять на простоfor button in buttons
.Ваш код — хороший пример того, как написать можно, но не нужно. Его нужно эволюционировать до чего-то более совершенного. В комментариях исправлять не получится, нужно видеть историю изменений.
Лучше залейте его на гитхаб, чтобы можно было его улучшать итеративно.
Советую почитать книгу "Чистый код" Роберта Мартина. Там, правда, не питон, но основные советы годятся.
Dendefo
Изначально некоторые циклы так и выглядели, но от этого пришлось отказаться, потому что мне нужен был индекс элемента в массиве, а не сам элемент. Хотя да, в одном моменте в
можно было написать короче.CJay
зачем вам методы setXXX и viewXXX? Почему не обращаться к полю непосредственно? В питоне же всё открыто, подобное сокрытие не имеет смысла.
И ещё, попробуйте отделить данные от представления. Пусть данные хранятся в массиве (списке) как целые числа. Откажитесь от класса Pole. Вам не нужно у каждой клетки иметь список соседних (это легко подсчитать и так). Как вариант — вообще отказаться от массива всех ячеек. Можно иметь лишь список с координатами бомб.
Да, и ещё. Всё-таки классы нужно называть с большой буквы.
Dendefo
Around как раз и подсчитывает, а делать это несколько раз в разных местах — глупо. Насчет циклов и set/view исправился. Ссылка на гит выше.
CJay
Да, посмотрел. Только вы в гист сохранили, а это, как я понял, сервис снипетов. Я же советую в сам github залить, чтобы разные версии коммитить.
Dendefo
Мне выше, на гист ссылку и дали, а повторять этот же код на самом гите не лучшая мысль, как мне кажется.
CJay
Почему не лучшая мысль?! Нормальная мысль.
Вы хотите узнать, как сделать этот код лучше?! Если да, то создавайте.
Dendefo
Звучит так, будто вы заманиваете меня на темную сторону силы. https://github.com/dendefo/MineSweeper А там есть печеньки?
CJay
Знания — однозначно сила. А вот в какой цвет вы эту силу окрасите, зависит от вас.
Завтра попробую пул реквесты поотправлять с предложениями об улучшении кода.
Думаю, мне удастся показать, как избавиться от globals и сделать код стройным и понятным не только интерпретатору.
ahmpro
для этого есть enumerate: