【エンジニアの道は果てしない】リーダブルコードを読んでリファクタリングを実践してみる
こんにちは。すうちです。
ここ数ヶ月ほど直接ＩＴ技術と関係ない話題が多かったですが、今回はガッツリＩＴ寄りプログラムの話です。
以前、エンジニア界隈で話題になった「リーダブルコード」を少し前に読みました。
多くの方が名著とおススメしてた通り、私もとても感銘を受けました。本を読んで頭では理解したつもりでしたが、せっかくなので何かの形で実践したいと思いました。
今回は「リーダブルコード」で学んだことを踏まえて、以前書いたコードをリファクタリングしてみた話です。
はじめに
リファクタリングとは、プログラムの外部仕様は変えず内部の構造やコードを整理することです。
以前Tkinterで作る画像編集ソフトの記事を書きました。
投稿の後半は急に忙しくなり、一応目的の形はできたもののコードの整理や見直しまで十分にできてませんでした。
という訳で、対象コードは以前作った上記をお題にしたいと思います（自分で自分のコードにつっこみ入れる形です…）。
修正方針は、タイトル通り「リーダブルコード」を参考にしました。
本には読みやすいコードを書くため様々な方法が紹介されていますが、今回は主に３つの観点で実施しています。
１．プログラムの見栄えを整える
２．関数（タスク）は一度に一つのことを
３．無駄なコードを減らす
前提のコードを少し補足します。
以前作成したTkinterの静止画/動画編集プログラムです。
クラスは以下の構成です。
１．プログラムの見栄えを整える
適切な関数、変数名に変更
まず始めに関数や変数名が適切か（名前が処理を表すか）確認してみます。
GetEventState関数
最初は、TkinterのGUI操作のイベント処理で至る所で呼ばれるコマンドと状態遷移の実行可否を判定するGetEventState関数です。
修正前（例：event_play）
def event_play(self):
if self.control.GetEventState('play'): # <= ココ
self.control.Video('play')
self.button_play['text'] = 'Stop'
elif self.control.GetEventState('stop'): # <= ココ
self.control.Video('stop')
self.button_play['text'] = 'Play'
print('{} :{}'.format(sys._getframe().f_code.co_name, self.button_play['text']))
この関数の戻り値は真偽値（True又はFalseを返す）ですが関数名はなぜかGetEventState()…?? 今更ですが本に倣って頭に真偽判定を意味する’Is’を付けて関数名をGetEventState()からIsTranferToState()に変更します。
修正後（例：event_play）
def event_play(self):
if self.control.IsTranferToState('play'): # <= ココ
self.control.Video('play')
self.button_play['text'] = 'Stop'
elif self.control.IsTranferToState('stop'): # <= ココ
self.control.Video('stop')
self.button_play['text'] = 'Play'
print('{} :{}'.format(sys._getframe().f_code.co_name, self.button_play['text']))
SetEventState関数
続いて動画再生時、経過時間の更新箇所。再生終了時に状態更新する関数SetEventState()です。マジックナンバー’２’が引数で、ぱっと見意味がわかりません。
修正前
def update_timestamp(self, status, frame_no, h,m,s):
canvas_id = int(self.select_canvas.replace('Video',''))
idx = canvas_id -1
if status:
self.label_frame[idx]['text'] = 'Time:{:02}:{:02}:{:02}'.format(h,m,s)
else:
self.button_play['text'] = 'Play'
self.label_frame[0]['text'] = 'Time:{:02}:{:02}:{:02}'.format(h,m,s)
self.label_frame[1]['text'] = 'Time:{:02}:{:02}:{:02}'.format(h,m,s)
self.control.SetEventState(2) # <= ココここは再生終了時に強制的に再生停止状態（値は２）に移行する意図でしたが、それに相当する名前ForceToState()に変更します。
修正後
else:
self.button_play['text'] = 'Play'
self.label_frame[0]['text'] = 'Time:{:02}:{:02}:{:02}'.format(h,m,s)
self.label_frame[1]['text'] = 'Time:{:02}:{:02}:{:02}'.format(h,m,s)
self.control.ForceToState('stop') # <= ココget_file関数
次にControlクラスで事前に取得したファイルリストからそのファイルのパスを取り出す関数get_file()。これの戻り値はファイル名ではなく、ファイルパス（ファイルがあるディレクトリ位置）なので、変数名をfnameからfile_pathに修正しました。
修正前
def Set(self, select_tab, set_pos, callback):
if select_tab == '[Photo]':
fname = self.get_file('set', set_pos) # <= ココ
self.model.DrawPhoto(fname, self.photo_canvas, 'None')
else:
fname = self.get_file('set', set_pos) # <= ココ
self.model.DeleteRectangle(self.canvas['Video1'])
self.model.DeleteRectangle(self.canvas['Video2'])
self.video_tag = 'Video1'
self.video_canvas = self.canvas[self.video_tag]
self.model.SetVideo(fname, self.video_canvas, self.video_tag, 'set', callback)
：修正後
def Set(self, select_tab, set_pos, callback):
if select_tab == '[Photo]':
file_path = self.get_file('set', set_pos) # <= ココ
self.model.DrawPhoto(file_path, self.canvas['Photo'], 'None')
else: # '[Video]'
file_path = self.get_file('set', set_pos) # <= ココ
self.model.DeleteRectangle(self.canvas['Video1'])
self.model.DeleteRectangle(self.canvas['Video2'])
self.video_tag = 'Video1'
self.model.SetVideo(file_path, self.canvas[self.video_tag], self.video_tag, 'set', callback)
：インデント調整（類似処理の並び）
こちらはコードの’＝’の位置や関数や変数の位置を類似処理毎に揃えました(ModelImage.pyを除く）。
修正前（例： GUIイベント、レイアウト配置 / ViewGUI.py）
# マウスイベント登録
self.window_sub_canvas.bind('<ButtonPress-1>', self.event_clip_start)
self.window_sub_canvas.bind('<Button1-Motion>', self.event_clip_keep)
self.window_sub_canvas.bind('<ButtonRelease-1>', self.event_clip_end)
self.window_video_canvas1.bind('<Double-Button-1>', self.event_mouse_select1)
self.window_video_canvas2.bind('<Double-Button-1>', self.event_mouse_select2)
self.window_video_canvas1.bind('<ButtonPress-1>', self.event_clip_start)
self.window_video_canvas1.bind('<Button1-Motion>', self.event_clip_keep)
self.window_video_canvas1.bind('<ButtonRelease-1>', self.event_clip_end)
self.window_video_canvas2.bind('<ButtonPress-1>', self.event_clip_start)
self.window_video_canvas2.bind('<Button1-Motion>', self.event_clip_keep)
self.window_video_canvas2.bind('<ButtonRelease-1>', self.event_clip_end)
## ウィジェット配置
# サブウィンドウ
self.window_sub_ctrl1.place(relx=0.68, rely=0.05)
self.window_sub_ctrl2.place(relx=0.68, rely=0.30)
self.window_sub_ctrl3.place(relx=0.30, rely=0.90)
self.window_sub_ctrl4.place(relx=0.25, rely=0.93)
self.window_sub_frame.place(relx=0.01, rely=0.01)
self.notebook.place(relx=0.001, rely=0.001)
# Photo[tab1]
self.window_sub_canvas.place(relx=0.09, rely=0.05)
# Video[tab2]
self.window_video_canvas1.place(relx=0.042, rely=0.01)
self.window_video_canvas2.place(relx=0.042, rely=0.44)
# window_sub_ctrl1
self.button_setdir.grid(row=1, column=1, padx=5, pady=5, sticky=tk.W)
self.entry_dir.grid(row=2, column=1, padx=5, pady=5, sticky=tk.W)
label_target.grid(row=3, column=1, padx=5, pady=5, sticky=tk.W)
self.combo_file.grid(row=4, column=1, padx=5, pady=5, sticky=tk.W)
# window_sub_ctrl2
label_s2_blk1.grid(row=1, column=1, padx=5, pady=5, sticky=tk.W)
label_rotate.grid(row=2, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[0].grid(row=3, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[1].grid(row=3, column=2, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[2].grid(row=3, column=3, padx=5, pady=5, sticky=tk.W)
label_flip.grid(row=4, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_flip[0].grid(row=5, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_flip[1].grid(row=5, column=2, padx=5, pady=5, sticky=tk.W)
label_clip.grid(row=6, column=1, padx=5, pady=5, sticky=tk.W)
button_clip_start.grid(row=7, column=1, padx=5, pady=5, sticky=tk.W)
button_clip_done.grid(row=7, column=2, padx=5, pady=5, sticky=tk.W)
label_run.grid(row=8, column=1, columnspan=2, padx=5, pady=5, sticky=tk.W)
button_undo.grid(row=9, column=1, padx=5, pady=5, sticky=tk.W)
button_save.grid(row=9, column=2, padx=5, pady=5, sticky=tk.W)
# window_sub_ctrl3
label_s3_blk1.grid(row=1, column=1, columnspan=2, padx=5, pady=5, sticky=tk.EW)
button_prev.grid(row=1, column=1, padx=5, pady=5, sticky=tk.E)
label_s3_blk2.grid(row=1, column=4, columnspan=2, padx=5, pady=5, sticky=tk.EW)
button_next.grid(row=1, column=3, padx=5, pady=5, sticky=tk.W)
# Video
# Button
self.button_play.grid(row=1, column=1, padx=5, pady=5, sticky=tk.E)
self.button_step.grid(row=1, column=2, padx=5, pady=5, sticky=tk.E)
self.button_capture.grid(row=1, column=3, padx=5, pady=5, sticky=tk.E)
self.button_speed.grid(row=1, column=4, padx=5, pady=5, sticky=tk.E)
#
self.label_frame[0].place(relx=0.44, rely=0.36)
self.label_frame[1].place(relx=0.44, rely=0.79)
label_barunit.place(relx=0.90, rely=0.85)
# Slide bar(Scale)
self.bar_scale.place(relx=0.08, rely=0.85)修正後（例： GUIイベント、レイアウト配置 / ViewGUI.py）
# マウスイベント登録
self.window_photo_canvas.bind ('<ButtonPress-1>', self.event_clip_start)
self.window_photo_canvas.bind ('<Button1-Motion>', self.event_clip_keep)
self.window_photo_canvas.bind ('<ButtonRelease-1>', self.event_clip_end)
self.window_video_canvas1.bind ('<Double-Button-1>', self.event_mouse_select1)
self.window_video_canvas2.bind ('<Double-Button-1>', self.event_mouse_select2)
self.window_video_canvas1.bind ('<ButtonPress-1>', self.event_clip_start)
self.window_video_canvas1.bind ('<Button1-Motion>', self.event_clip_keep)
self.window_video_canvas1.bind ('<ButtonRelease-1>', self.event_clip_end)
self.window_video_canvas2.bind ('<ButtonPress-1>', self.event_clip_start)
self.window_video_canvas2.bind ('<Button1-Motion>', self.event_clip_keep)
self.window_video_canvas2.bind ('<ButtonRelease-1>', self.event_clip_end)
## ウィジェット配置
# サブウィンドウ
self.window_sub_ctrl1.place (relx=0.68, rely=0.05)
self.window_sub_ctrl2.place (relx=0.68, rely=0.30)
self.window_sub_ctrl3.place (relx=0.30, rely=0.90)
self.window_sub_ctrl4.place (relx=0.25, rely=0.93)
self.window_sub_frame.place (relx=0.01, rely=0.01)
self.notebook.place (relx=0.001, rely=0.001)
# Photo[tab1]
self.window_photo_canvas.place (relx=0.09, rely=0.05)
# Video[tab2]
self.window_video_canvas1.place (relx=0.042, rely=0.01)
self.window_video_canvas2.place (relx=0.042, rely=0.44)
# window_sub_ctrl1
self.button_setdir.grid (row=1, column=1, padx=5, pady=5, sticky=tk.W)
self.entry_dir.grid (row=2, column=1, padx=5, pady=5, sticky=tk.W)
label_target.grid (row=3, column=1, padx=5, pady=5, sticky=tk.W)
self.combo_file.grid (row=4, column=1, padx=5, pady=5, sticky=tk.W)
# window_sub_ctrl2
label_s2_blk1.grid (row=1, column=1, padx=5, pady=5, sticky=tk.W)
label_rotate.grid (row=2, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[0].grid (row=3, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[1].grid (row=3, column=2, padx=5, pady=5, sticky=tk.W)
self.btn_rotate[2].grid (row=3, column=3, padx=5, pady=5, sticky=tk.W)
label_flip.grid (row=4, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_flip[0].grid (row=5, column=1, padx=5, pady=5, sticky=tk.W)
self.btn_flip[1].grid (row=5, column=2, padx=5, pady=5, sticky=tk.W)
label_clip.grid (row=6, column=1, padx=5, pady=5, sticky=tk.W)
button_clip_start.grid (row=7, column=1, padx=5, pady=5, sticky=tk.W)
button_clip_done.grid (row=7, column=2, padx=5, pady=5, sticky=tk.W)
label_run.grid (row=8, column=1, columnspan=2, padx=5, pady=5, sticky=tk.W)
button_undo.grid (row=9, column=1, padx=5, pady=5, sticky=tk.W)
button_save.grid (row=9, column=2, padx=5, pady=5, sticky=tk.W)
# window_sub_ctrl3
label_s3_blk1.grid (row=1, column=1, columnspan=2, padx=5, pady=5, sticky=tk.EW)
button_prev.grid (row=1, column=1, padx=5, pady=5, sticky=tk.E)
label_s3_blk2.grid (row=1, column=4, columnspan=2, padx=5, pady=5, sticky=tk.EW)
button_next.grid (row=1, column=3, padx=5, pady=5, sticky=tk.W)
# Video
# Button
self.button_play.grid (row=1, column=1, padx=5, pady=5, sticky=tk.E)
self.button_step.grid (row=1, column=2, padx=5, pady=5, sticky=tk.E)
self.button_capture.grid (row=1, column=3, padx=5, pady=5, sticky=tk.E)
self.button_speed.grid (row=1, column=4, padx=5, pady=5, sticky=tk.E)
#
self.label_frame[0].place(relx=0.44, rely=0.36)
self.label_frame[1].place(relx=0.44, rely=0.79)
label_barunit.place (relx=0.90, rely=0.85)
# Slide bar(Scale)
self.bar_scale.place (relx=0.08, rely=0.85)インデントは全体的に調整必要なので手間と思える所もありますが、実際やってみるとやはり修正後の方が見た目は良いですね。
２．関数は一度に一つのことを
目的が異なる複数の処理を分離
SetDirlist関数
この関数は引数指定のディレクトリにあるファイル（例：.jpg、.mp4等）を取得しクラスの内部変数self.target_files[self.select_tab]に保存します。
ただ、よく見ると（よく見なくても…）関数の戻り値は二つあり、前半はファイルリストを作る処理。後半は現在のファイルパス（最終的にはファイル名）を取得してます。
修正前
def SetDirlist(self, dir_path):
self.dir_path = dir_path
tab = self.select_tab
target_files = []
file_list = os.listdir(self.dir_path)
target_ext = self.ext_keys[self.select_tab]
print(tab, target_ext)
for fname in file_list:
if self.is_target(fname, target_ext):
target_files.append(fname)
self.target_files[self.select_tab] = target_files
cur_file = 'None'
if len(target_files) > 0:
cur_file = self.get_file('current')
print(cur_file)
return self.target_files[self.select_tab], os.path.basename(cur_file)
’関数は一度に一つのことを’に則って、関数を２つに分離したいと思います。
ちなみに元の関数名はSetDirlist()ですが、前述の説明通りディレクトリではなくファイルリストの取得なので明らかに命名がおかしいですね（なぜ今まで気づかなかったのか…）。なので関数名も修正します。
修正後
def SetFilelist(self, dir_path):
self.dir_path = dir_path
tab = self.select_tab
target_files = []
file_list = os.listdir(self.dir_path)
target_ext = self.ext_keys[self.select_tab]
print(tab, target_ext)
for file_name in file_list:
if self.is_target(file_name, target_ext):
target_files.append(file_name)
self.target_files[self.select_tab] = target_files
return self.target_files[self.select_tab]
def GetCurrentFile(self):
target_files = self.target_files[self.select_tab]
if len(target_files) > 0:
file_path = self.get_file('current')
return os.path.basename(file_path)
else:
return 'None'３．無駄なコードを減らす
SetDirlistの反映
SetDirlist()の修正とあわせて関数呼び出し部分を見直します。まず例のevent_tabchanged()では取得ファイルのリストをself.file_listに保存しますが、これはTkinterのコンボBOX self.combo_file['value']に設定し直すだけの不要な変数です。
また後半の条件分岐は、GetCurrentState()の結果から表示文字列を変えますが cur_state == 0は対象ファイルがない状態を意味するので、前述のGetCurrentFile()の処理と重複するため、戻り値（ファイル名）をそのまま設定すれば問題ないと判断しました。
修正前（例：event_tabchanged、event_set_folder）
def event_tabchanged(self, event):
notebook = event.widget
self.select_tab = notebook.tab(notebook.select(), 'text')
self.control.SetTab(self.select_tab)
self.file_list, fname = self.control.SetDirlist(self.dir_path)
self.combo_file['value'] = self.file_list
cur_state = self.control.GetCurrentState()
if cur_state == 0:
self.combo_file.set('..Choose file')
else:
self.combo_file.set(fname)
print('{}: {}'.format(sys._getframe().f_code.co_name, self.select_tab))
def event_set_folder(self):
if self.control.GetEventState('dir'):
print(sys._getframe().f_code.co_name)
self.dir_path = filedialog.askdirectory(initialdir=self.dir_path, mustexist=True)
self.str_dir.set(self.dir_path)
self.file_list, fname = self.control.SetDirlist(self.dir_path)
self.combo_file['value'] = self.file_list
cur_state = self.control.GetCurrentState()
if cur_state == 0:
self.combo_file.set('..Choose file')
else:
self.combo_file.set(fname)以下、前述の方針で修正したコードです。あわせてGetCurrentState()は不要になったので削除しました。
修正後（例：event_tabchanged、event_set_folder）
def event_tabchanged(self, event):
notebook = event.widget
self.select_tab = notebook.tab(notebook.select(), 'text')
self.control.SetTab(self.select_tab)
self.combo_file['value'] = self.control.SetFilelist(self.dir_path)
file_name = self.control.GetCurrentFile()
self.combo_file.set(file_name)
print('{}: {}'.format(sys._getframe().f_code.co_name, self.select_tab))
def event_set_folder(self):
if self.control.IsTranferToState('dir'):
print(sys._getframe().f_code.co_name)
self.dir_path = filedialog.askdirectory(initialdir=self.dir_path, mustexist=True)
self.str_dir.set(self.dir_path)
self.combo_file['value'] = self.control.SetFilelist(self.dir_path)
file_name = self.control.GetCurrentFile()
self.combo_file.set(file_name)ちなみに、self.control.GetCurrentFile()のfile_nameは本当は不要ですが、戻り値の意味を残す目的でこの形にしました。
動画/静止画キャンバス保存変数（photo_canvas 、video_canvas）
動画/静止画のキャンバス（Tkinterの画像表示領域）の変数としてself.photo_canvas、 self.video_canvasを用意しましたが、同じ情報はself.canvasにも保存してます。
下記 Set()のように実際使用しているのはself.canvas['キャンバス名']です。元々静止画はキャンバス１面で切替なし（そもそもphoto_canvas変数は不要）。
動画は２面ありますが使用キャンバス名はself.video_tagに保存されているので、self.canvas[self.video_tag]により現在アクティブなキャンバスを知ることができます。
これらの理由からphoto_canvas 、video_canvasは不要なので削除します。
修正前（例：InitCanvas、Set）
def InitCanvas(self, window_canvas_dict):
for ks, canvas in window_canvas_dict.items():
self.canvas[ks] = canvas
self.photo_canvas = self.canvas['Photo']
self.video_canvas = self.canvas['Video1']
self.frame['Video1'] = 0
self.frame['Video2'] = 0
def Set(self, select_tab, set_pos, callback):
if select_tab == '[Photo]':
fname = self.get_file('set', set_pos)
self.model.DrawPhoto(fname, self.photo_canvas, 'None')
else:
fname = self.get_file('set', set_pos)
self.model.DeleteRectangle(self.canvas['Video1'])
self.model.DeleteRectangle(self.canvas['Video2'])
self.video_tag = 'Video1'
self.video_canvas = self.canvas[self.video_tag]
self.model.SetVideo(fname, self.video_canvas, self.video_tag, 'set', callback)
_, self.frame['Video1'] = self.model.GetVideo('status')
_, self.frame['Video2'] = self.model.GetVideo('status')
print('tag, fno1, fno2',self.video_tag, self.frame['Video1'], self.frame['Video2'])以下、修正コードです。
双方のキャンバスに表示中のフレーム番号を保存するself.frame['キャンバス名']は、動画ファイル選択時 Set()で初期化されるのでInitCanvas()では不要（本来ここでの初期化は不適切）なのであわせて削除します。
修正後（例：InitCanvas、Set）
def InitCanvas(self, window_canvas_dict):
for ks, canvas in window_canvas_dict.items():
self.canvas[ks] = canvas
def Set(self, select_tab, set_pos, callback):
if select_tab == '[Photo]':
file_path = self.get_file('set', set_pos)
self.model.DrawPhoto(file_path, self.canvas['Photo'], 'None')
else: # '[Video]'
file_path = self.get_file('set', set_pos)
self.model.DeleteRectangle(self.canvas['Video1'])
self.model.DeleteRectangle(self.canvas['Video2'])
self.video_tag = 'Video1'
self.model.SetVideo(file_path, self.canvas[self.video_tag], self.video_tag, 'set', callback)
_, self.frame['Video1'] = self.model.GetVideo('status')
_, self.frame['Video2'] = self.model.GetVideo('status')
print('tag, fno1, fno2',self.video_tag, self.frame['Video1'], self.frame['Video2'])４．全体のソースコード（修正後）
その他の修正もありますが、本記事では割愛します。
コード修正後は、【PythonでGUIを作りたい】Tkinterで作る画像編集ソフト＃９：動画編集の実装とテストと同等動作する所まで確認済です。
もし修正コードにご興味ある方は、下記リンクを参照頂ければと思います。
GUI Image Editor(コード一式)
https://github.com/suti-hub/GUI_Image_Editor
最後に
今回の修正は内部構造は基本そのままに比較的簡単にできる範囲に留めましたが、これで正解という意図はありません。
その意味では、新人の頃にお世話になった方の言葉「ソフトウェアに正解はないが、明らかな間違いはある」を思い出します。
少なくとも修正前のコードは間違いの部類に入ると思いますが、修正は他にも良いやり方はあるかもしれません。
加えてその方が言ってた「ソフトウェアは大体３回書き直すと理想の形に近づく」という話も思い出しましたが、私も１回で理想のプログラムを書けたことはありません。一度書いて見直すべき点に気づくのが多いです。
「リーダブルコード」を参考にコードを修正してみて、本の内容は読みやすいコードを書く手助けになると改めて思いました。
Tkinterで作る画像編集ソフトを終えてから３か月近く経ちますが、最近少し余裕できたこの機会に当時できなかったコード整理をやれて少しスッキリしました。
最後まで読んで頂き、ありがとうございました。