Python学習を始めてから一か月が経ち、集大成として"スッキリわかるPython入門"の最終課題であるパズルRPGの制作が終わりました。完成後、複雑であった関数のコードをリファクタリングして初心者卒業の一歩を踏み出しました。
このゲームは表示される宝石の記号をアルファベットを入力して動かし３つ以上隙間なく同じ種類の宝石が並ぶと消えて、計算された分のダメージがモンスターに加わるといったものです。興味がある人は上のリンクからgithubでコードをダウンロードしてみてください。
今回は実際に作った２つの関数についてみてみましょう。
一つ目はゲームの中で３つ以上並んだ宝石を消さなければいけないのですが、その宝石を特定する関数check_banishable()です。最初に私が書いたコードがこちら
def check_banishable(jewel_dict):
jewel_list = list(jewel_dict.values())
banish_list = list()
banish_small_list = list()
for num in range(len(jewel_list)-1):
if jewel_list[num] == jewel_list[num+1] and num != len(jewel_list)-2:
banish_small_list.append(num)
banish_small_list.append(num+1)
elif num != len(jewel_list)-2:
banish_dict = dict.fromkeys(banish_small_list)
banish_small_list = list(banish_dict)
if len(banish_small_list) >= 3:
for small in banish_small_list:
banish_list.append(small)
break
else:
banish_small_list = []
elif num == len(jewel_list)-2:
if jewel_list[num] == jewel_list[num+1]:
banish_small_list.append(num)
banish_small_list.append(num+1)
else:
banish_small_list.append(num)
banish_dict = dict.fromkeys(banish_small_list)
banish_small_list = list(banish_dict)
if len(banish_small_list) >= 3:
for small in banish_small_list:
banish_list.append(small)
return banish_list
かなりコードが長く複雑な分岐と繰り返しが含まれていることがわかります。このコードは実際に動きますが、複雑なコードだとバグが生まれやすくなったり、変更を加える際に手間になってしまいます。そこで以下のようにリファクタリングしました。
def check_banishable(jewel_list):
banish_list = []
total = len(jewel_list)
for i in range(total-2):
if jewel_list[i] == jewel_list[i+1] == jewel_list[i+2]:
current_jewel = jewel_list[i]
end_i = i + 2
for j in range(i + 3, total):
if jewel_list[j] == current_jewel:
end_i = j
else:
break
banish_list = list(range(i, end_i+1))
break
return banish_list
だいぶコードが簡単になり、スッキリしましたよね。最初のコードでは入力されたアルファベットで直接アクセスできるようにディクショナリで宝石が管理されていました。リファクタリング後はインデックスで管理できるように引数がリストになっています。また、連続した宝石を探す際に最初のコードでは隣り合う宝石を一つずつチェックしていましたが、３つ以上連続した宝石を最初から探すようにしたことで分岐をより少なくすることができました。
二つ目は消した宝石を左にすべて寄せる関数shift_gems()です。
def shift_gems(jewel_dict):
jewel_list = list(jewel_dict.values())
check = True
while check:
if not {"無":" "} in jewel_list:
check = False
else:
index_0 = jewel_list.index({"無":" "})
for i in range(index_0+1, 14):
if jewel_list[i] == {"無":" "} and i != 13:
continue
elif i != 13:
dewel_copy1 = jewel_list[index_0].copy()
dewel_copy2 = jewel_list[i].copy()
jewel_list[index_0] = dewel_copy2
jewel_list[i] = dewel_copy1
jewel_dict = dict(zip(list(jewel_dict), jewel_list))
gems_show(jewel_dict)
jewel_list = list(jewel_dict.values())
break
elif i == 13 and jewel_list[i] == {"無":" "}:
check = False
else:
dewel_copy1 = jewel_list[index_0].copy()
dewel_copy2 = jewel_list[i].copy()
jewel_list[index_0] = dewel_copy2
jewel_list[i] = dewel_copy1
jewel_dict = dict(zip(list(jewel_dict), jewel_list))
gems_show(jewel_dict)
jewel_list = list(jewel_dict.values())
check = False
jewel_dict = dict(zip(list(jewel_dict), jewel_list))
return jewel_dict
こちらもなかなか長いですね。先ほどのように宝石をディクショナリで管理している点や、同じコードの羅列がコードを長く複雑にしています。
def shift_gems(jewel_list):
check = True
while check:
if not {"無":" "} in jewel_list:
check = False
else:
index_0 = jewel_list.index({"無":" "})
for i in range(index_0+1, 14):
if jewel_list[i] == {"無":" "} and i != 13:
continue
elif i != 13:
jewel_list = switch_gem(index_0, i, jewel_list)
break
elif i == 13 and jewel_list[i] == {"無":" "}:
check = False
else:
jewel_list = switch_gem(index_0, i, jewel_list)
check = False
return jewel_list
リストにしたとこでシンプルになったのと、同じコードの羅列部分をswitch_gem()という新しい関数を作ってそれをコード内で使うことでわかりやすくなっています。関数はなるべく一つの仕事だけをさせるようにすることでバグのリスクを下げられます。
同じ働きをするコードでもシンプルでわかりやすいコードは好まれることが分かりますね。リファクタリングは面倒に思えますが大切だということを知りました！