みなさんこんにちは。延々迷っていた3Dプリンタを遂に購入しました。ちょうど何度目かの3Dプリンタほしい病がピークに達していたところにFlashforge Adventurer3用高温対応ノズルが発売されて、コンパクトな筐体サイズ＆高温対応という正に私がほしかった要件を満たす組合せが実現したので即断即決でした。おかげで最近は3Dプリンタ遊びとホームセンター通いがはかどっています。
TEXT＿痴山紘史 / Hiroshi Chiyama（日本CGサービス）
EDIT＿尾形美幸 / Miyuki Ogata（CGWORLD）
より良いプログラムを書くには？
前回は、より良いプログラムを書くための準備としてコーディングガイドラインの策定、チェック体制の用意、テストコードの作成、パッケージ化といったトピックをご紹介しました。今回は、「良いプログラムを書くってどういうこと？」という内容を、実際のコードを題材にご紹介します。
前回触れたように、作成されたプログラムはプロジェクトの寿命よりも長く、場合によっては複数人の手によって10年以上も生き続けます。そのような寿命の長いプログラムの場合、プログラムを書く時間よりもそれをメンテナンスするための時間の方が圧倒的に長くなります。そのため、コードを書く際には人が読んで読みやすい、読んだときに処理内容を誤解しにくいコードを書くということがとても大事です。読みやすいコードは後からコードを読むときのコストを減らすだけではなく、コードを書いている途中でも処理内容を簡単に把握し、脳の使用メモリを削減し、バグが紛れることを防ぐ効果があります。最初はコードの読みやすさに注目してコードを書き、パフォーマンスの問題が出たときに問題の部分をチューニングするという順番で行うのが王道です。
既存コードのリファクタリング
読みやすいコードとそうでないコードの比較を行うために、既存のコードをリファクタリングしていきます。元になるのは collect_filenames_from_dir という関数で、元は以下のようになっています。 この中で使用されている self.file_pat は 事前に re.compile(u'([^_]+_[^_]+_[^_.]+).*') として定義されています。まずはご自分でこのコードの処理を追いかけてみてください。
1 def collect_filenames_from_dir(self, directory):
2 print(u'\nphase 1: find files in uploading directory...\n')
3 ret = {}
4 ep_sq_sh_map = {}
5 result_map = {}
6 dir_abs = unicode(directory, 'cp932')
7 for root, dirs, files in os.walk(dir_abs, topdown=True):
8 file_names = [(os.path.join(root, name), name) for name in files]
9 for x in file_names:
10 m = self.file_pat.match(x[1])
11 if m is not None:
12 ep_sq_sh = m.group(1)
13 ent = ret.get(x[1], None)
14 if ent is None:
15 try:
16 ascii_file = x[0].encode('ascii')
17 except:
18 print(u'[FAIL ][%s] can not upload a file with international file name.' % x[1])
19 result_map[x[1]] = 'ILLEGALFILENAME'
20 else:
21 ret[x[1]] = (ep_sq_sh, x[0])
22 ep_sq_sh_map[ep_sq_sh] = 1
23 else:
24 # duplicate same file name in different dirs: simply use first one!
25 print(u'[WARN ][%s] same file name in this upload folder! (found %s and ignored!)' %
26 (ent[0], x[0]))
27 else:
28 pass
29 # not match file template...simply ignored!
30
31 return ret, ep_sq_sh_map, result_map
いかがでしょう。それほど長くない関数ですが、何だかゴチャッとしているなという印象を受けます。これを順番に整理していきます。
判定条件を組み替える
オリジナルのコードを見たとき、まず最初に気になるのがインデントの深さです。try/except が含まれているとはいえ、この長さの関数で5段階のインデントになっているのは深いなという印象です。
判定条件や順番を組み替えることでインデントの深さやコードの読みやすさはグっと改善されることが多々あります。たとえば、
if 条件:
処理
else:
continue
と
if not 条件:
continue
処理
は等価です。後者の場合、処理に関係ない部分を事前に刈り取っているため、処理を行う際に覚えておかないといけない内容が減ります。また、処理本体のインデントも浅くなるためにコードもスッキリします。
これと同じことを行います。
1 def collect_filenames_from_dir(self, directory):
2 print(u'\nphase 1: find files in uploading directory...\n')
3
4 ret = {}
5 ep_sq_sh_map = {}
6 result_map = {}
7 dir_abs = unicode(directory, 'cp932')
8
9 for root, dirs, files in os.walk(dir_abs, topdown=True):
10 file_names = [(os.path.join(root, name), name) for name in files]
11 for x in file_names:
12 m = self.file_pat.match(x[1])
13 if m is None:
14 # not match file template...simply ignored!
15 continue
16
17 ep_sq_sh = m.group(1)
18 ent = ret.get(x[1], None)
19 if ent is not None:
20 # duplicate same file name in different dirs: simply use first one!
21 print(u'[WARN ][%s] same file name in this upload folder! (found %s and ignored!)' %
22 (ent[0], x[0]))
23 continue
24
25 try:
26 ascii_file = x[0].encode('ascii')
27 except:
28 print(u'[FAIL ][%s] can not upload a file with international file name.' % x[1])
29 result_map[x[1]] = 'ILLEGALFILENAME'
30 else:
31 ret[x[1]] = (ep_sq_sh, x[0])
32 ep_sq_sh_map[ep_sq_sh] = 1
33
34 return ret, ep_sq_sh_map, result_map
これだけでインデントの深さが5段から3段に改善されました。また、13行目と19行目で、処理条件に合わないものは即座にパスするようにしています。そのため、処理が必要な部分については25～32行目に集中すればよくなっています。
変数名のみなおし
ループで使用されている x という変数が各所で多用されているにも関わらず中身はタプルで、一見して何がどうなっているのかわかり辛くなってしまっています。 ここは一見してわかりやすい変数名をつけるのがいいでしょう。また、ent という変数も存在意義がないので削除します。
1 def collect_filenames_from_dir(self, directory):
2 print(u'\nphase 1: find files in uploading directory...\n')
3
4 ret = {}
5 ep_sq_sh_map = {}
6 result_map = {}
7 dir_abs = unicode(directory, 'cp932')
8
9 for root, dirs, files in os.walk(dir_abs, topdown=True):
10 file_names = [(os.path.join(root, name), name) for name in files]
11 for full_path, file_name in file_names:
12 m = self.file_pat.match(file_name)
13 if m is None:
14 # not match file template...simply ignored!
15 continue
16
17 ep_sq_sh = m.group(1)
18 if file_name in ret:
19 # duplicate same file name in different dirs: simply use first one!
20 print(u'[WARN ][%s] same file name in this upload folder! (found %s and ignored!)' %
21 (ret[file_name], full_path))
22 continue
23
24 try:
25 ascii_file = full_path.encode('ascii')
26 except:
27 print(u'[FAIL ][%s] can not upload a file with international file name.' % file_name)
28 result_map[file_name] = 'ILLEGALFILENAME'
29 else:
30 ret[file_name] = (ep_sq_sh, full_path)
31 ep_sq_sh_map[ep_sq_sh] = 1
32
33 return ret, ep_sq_sh_map, result_map
処理のブロック化
ループが二重になっており、更にループ内で
-ファイルリストの取得
- ファイル名の重複チェック
-ファイルの整理
- result_map や ep_sq_sh_map の生成
という複数の処理が一度に行われてしまっています。これらを簡単な機能に分解して、その組み合わせで目的の処理を行うことができるようにします。
1 def collect_filenames_from_dir(self, directory):
2 print(u'\nphase 1: find files in uploading directory...\n')
3
4 dir_abs = unicode(directory, 'cp932')
5
6 # ファイルリストの生成
7 file_list = []
8 for root, dirs, files in os.walk(dir_abs, topdown=True):
9 file_list.extend([os.path.join(root, name) for name in files])
10
11 # ファイル名でまとめる
12 file_map = {}
13 for full_path in file_list:
14 file_name = os.path.basename(full_path)
15
16 if file_name not in file_map:
17 file_map[file_name] = [full_path]
18 else:
19 file_map[file_name].append(full_path)
20
21 # 本処理
22 ret = {}
23 ep_sq_sh_map = {}
24 result_map = {}
25
26 for file_name, files in file_map.iteritems():
27 # 対象外なファイルをフィルタリング
28 m = self.file_pat.match(file_name)
29 if m is None:
30 # not match file template...simply ignored!
31 continue
32
33 if len(files) != 1:
34 # ファイル名の重複チェック
35 print(u'[WARN ][%s] same file name in this upload folder!' % file_name)
36 print(u' ' + u' '.join(files))
37 continue
38
39 full_path = files[0]
40 ep_sq_sh = m.group(1)
41 try:
42 ascii_file = full_path.encode('ascii')
43 except:
44 print(u'[FAIL ][%s] can not upload a file with international file name.' % file_name)
45 result_map[file_name] = 'ILLEGALFILENAME'
46 else:
47 ret[file_name] = (ep_sq_sh, full_path)
48 ep_sq_sh_map[ep_sq_sh] = 1
49
50 return ret, ep_sq_sh_map, result_map
一見するとコードが長くなってしまいましたが、ファイルのリストアップやファイル名を元にした辞書の作成といった予備処理が分離されたことで、本当に行わなければいけない処理内容が凝縮されています。 処理のコアな部分が凝縮されることで、これまであまり気にならなかった部分が気になってくるようになります。
例えば、今のコードだと対象外なファイルのフィルタリングや重複チェックも別のブロックにできるはずですが、file_map からそのデータを削除し、エラー情報として別にまとめる必要がありそうです。 また、result_map や ep_sq_sh_map の存在も何だか気持ち悪いです。対象外なファイルや重複ファイルは無視してしまって、マルチバイトなファイル名は ILLEGALFILENAME として結果を返しているのも気になります。ep_sq_sh_map はフラグしか保持していないですが、これももうちょっと良い方法がありそうです。
エラー処理の整理
エラーの内容によってマチマチだった部分を整理します。
1 def collect_filenames_from_dir(self, directory):
2 print(u'\nphase 1: find files in uploading directory...\n')
3
4 errors = []
5 dir_abs = unicode(directory, 'cp932')
6
7 # ファイルリストの生成
8 file_list = []
9 for root, dirs, files in os.walk(dir_abs, topdown=True):
10 file_list.extend([os.path.join(root, name) for name in files])
11
12 # ファイル名でまとめる
13 file_map = {}
14 for full_path in file_list:
15 file_name = os.path.basename(full_path)
16 # 対象外なファイルをフィルタリング
17 m = self.file_pat.match(file_name)
18 if m is None:
19 erros.append(['not match file template', [full_path]])
20 continue
21
22 # マルチバイト文字列が使用されたファイルをフィルタリング
23 try:
24 ascii_file = full_path.encode('ascii')
25 except:
26 errors.append(['can not upload a file with international file name.', [full_path]])
27 continue
28
29 if file_name not in file_map:
30 file_map[file_name] = [full_path]
31 else:
32 file_map[file_name].append(full_path)
33
34 # 重複ファイルを排除
35 file_list = []
36 for file_name, files in file_map.iteritems():
37 if len(files) != 1:
38 # ファイル名の重複チェック
39 errors.append['[%s] same file name in this upload folder!' % file_name, files]
40 continue
41
42 file_list.append(files[0])
43
44 # 本処理
45 filenames = {}
46 ep_sq_sh_map = {}
47 for full_path in file_list:
48 file_name = os.path.basename(full_path)
49 m = self.file_pat.match(file_name)
50 ep_sq_sh = m.group(1)
51 filenames[file_name] = (ep_sq_sh, full_path)
52
53 if ep_sq_sh not in ep_sq_sh_map:
54 ep_sq_sh_map[ep_sq_sh] = [full_path]
55 else:
56 ep_sq_sh_map[ep_sq_sh].append(full_path)
57
58 return filenames, ep_sq_sh_map, errors
本処理を44～56行目に凝縮することができました。これなら本処理の内容を勘違いすることもないでしょう。
また、処理を行わなかったファイルの情報も errors としてまとめてあります。ep_sq_sh_map もフラグではなくファイルのフルパスを格納するように改良しました。細かいところですが、ret という名前も関数名に合わせて filenames に変更しています。
コードの量としては38行から58行に増えていますが、処理の内容や使用している変数が細かいブロック単位に分かれているため、コードを読む際に意識しなければいけない範囲が格段に減っています。また、処理をブロックごとに分けることでそのコードが何をしているのかを簡潔に表すことができるようになっています。今回はわかりやすさを優先するためにコメントを記入してありますが、実際にはコードを読めば一目で把握できる程度の処理がほとんどなのでコメントも必要ないでしょう。
ここまでコードを整理すると、次に何をどのようにすれば良いのかという道筋が見えてきます。また、それによって関数やクラス、アプリケーション全体をどのように設計すれば良いのか見えてくることもあります。変数名が使いまわされているのが賛否あるところですが、変数を使用する範囲が処理のブロック内で閉じているためひとまず良しとしておきます。命名については collect_filenames_from_dir や ep_sq_sh_map という名前はもうちょっと一工夫できそうな感じがします。
まとめ
今回は具体的な例を使って、あまりメンテナンス性の高くないコードを徐々に整理し、それぞれの段階でコードがどのように変化していくかを追いかけてみました。それぞれの段階でやっていることはとても単純で、良い命名、処理内容の分解、ながれの整理を淡々と行なっているだけです。新規にコードを書き起こすときもこれらを意識するだけで格段にメンテナンスしやすいコードを書くことができるでしょう。
第35回の公開は、2021年7月を予定しております。
プロフィール
