なんか一度こういうのやってみたかった
前提
-
csv.pyという csv っぽいlistをよしなにするモジュールを作ってみた、というところから - このモジュールは生まれたてで、まだまだ再設計する時間とリソースの余裕がある
- ファイル入出力は面倒なので
f.pyというモジュールで適当に済ます- 本当は
pathとかも必要だけど、そこも適当
- 本当は
- 言語はなんでも良かったんだけど、最近書いてないなーとふと思ったので python にした
- お題と言語はあくまでイメージであり、お題自体に対する突っ込みやイケてる python かどうか等は興味がない
初版
コード
f.py
def write(lines):
print 'write:', lines
csv.py
# -*- coding: utf-8 -*-
import f
def as_kvs(rows):
keys = rows[0]
value_rows = rows[1:]
return [dict(zip(keys, value_row)) for value_row in value_rows]
def write_first_one_or_empty(rows, sort_key, filter_key, filter_value, write_key):
# 複数の kv にする
kvs = as_kvs(rows)
# 指定のキーでソートする
_sorted = sorted(kvs, key=lambda row: row[sort_key])
# 指定のキーが指定の値の箇所だけにフィルターする
_filtered = filter(lambda row: row[filter_key] == filter_value, _sorted)
if 0 < len(_filtered):
# 先頭行を指定のキーで抽出する
_mapped_one = map(lambda row: row[write_key], _filtered)[0]
# 書き込む
f.write([_mapped_one])
else:
# 空ファイル作成
f.write([])
def write_all_or_empty(rows, filter_key, filter_value, write_key):
# 複数の kv にする
kvs = as_kvs(rows)
# 指定のキーが指定の値の箇所だけにフィルターする
_filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
if _filtered:
# 全行を指定のキーで抽出する
_mapped = map(lambda row: row[write_key], _filtered)
# 書き込む
f.write(_mapped)
else:
# 空ファイル作成
f.write([])
def write_all_or_error(rows, filter_key, filter_value, write_key):
# 複数の kv にする
kvs = as_kvs(rows)
# 指定のキーが指定の値の箇所だけにフィルターする
_filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
if _filtered:
# 全行を指定のキーで抽出する
_mapped = map(lambda row: row[write_key], _filtered)
# 書き込む
f.write(_mapped)
else:
# エラー
raise Exception("no result")
呼び元
main.py
# status, code からなる csv を与え
# code でソートして
# status が active の最初の一件の
# code を書き出す
# なければファイル作成だけする
csv.write_first_one_or_empty(
[['status', 'code'], ['dead', '001'], ['active', '003'], ['active', '002']],
'code', 'status', 'active', 'code'
)
# 結果
# write: ['002']
main.py
# name, gender からなる csv を与え
# gender が male の全件の
# name を書き出す
# なければファイル作成だけする
csv.write_all_or_empty(
[['name', 'gender'], ['Eva', 'female'], ['Sunny', 'female']],
'gender', 'male', 'name'
)
# 結果
# write: []
main.py
# status, tel からなる csv を与え
# status が dead な全件の
# tel を書き出す
# なければエラー
csv.write_all_or_error(
[['status', 'tel'], ['dead', '090-1111-1111'], ['active', '090-9999-9999']],
'status', 'dead', 'tel'
)
# 結果
# write: ['090-1111-1111']
問題
- 読みづらい
- 共通化されていない
- 今後に他メソッドを足す時に全コピーになってしまう
- 同じコードの部分に修正が入ったら、何カ所直さなければいけないか分からない
- コメントから得られる情報がない
- コードを読めば分かることしか書いてない
step-1 とりあえずコメントを英語で書いてみる
差分
def write_first_one_or_empty(rows, sort_key, filter_key, filter_value, write_key):
- # 複数の kv にする
+ # as kvs
kvs = as_kvs(rows)
- # 指定のキーでソートする
+ # sort by specified key
_sorted = sorted(kvs, key=lambda row: row[sort_key])
- # 指定のキーが指定の値の箇所だけにフィルターする
+ # filter by specified key and value
_filtered = filter(lambda row: row[filter_key] == filter_value, _sorted)
if 0 < len(_filtered):
- # 先頭行を指定のキーで抽出する
+ # extract by specified key and first one
_mapped_one = map(lambda row: row[write_key], _filtered)[0]
- # 書き込む
+ # write
f.write([_mapped_one])
else:
- # 空ファイル作成
+ # write empty
f.write([])
def write_all_or_empty(rows, filter_key, filter_value, write_key):
- # 複数の kv にする
+ # as kvs
kvs = as_kvs(rows)
- # 指定のキーが指定の値の箇所だけにフィルターする
+ # filter by specified key and value
_filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
if _filtered:
- # 全行を指定のキーで抽出する
+ # extract by specified key
_mapped = map(lambda row: row[write_key], _filtered)
- # 書き込む
+ # write
f.write(_mapped)
else:
- # 空ファイル作成
+ # write empty
f.write([])
def write_all_or_error(rows, filter_key, filter_value, write_key):
- # 複数の kv にする
+ # as kvs
kvs = as_kvs(rows)
- # 指定のキーが指定の値の箇所だけにフィルターする
+ # filter by specified key and value
_filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
if _filtered:
- # 全行を指定のキーで抽出する
+ # extract by specified key
_mapped = map(lambda row: row[write_key], _filtered)
- # 書き込む
+ # write
f.write(_mapped)
else:
- # エラー
+ # error
raise Exception("no result")
次の問題
-
# as kvsとか# writeとかあほくさいコメントがある - 情報量が変わっていないのに記載が多いのは二重管理でイケてない
step-2 英語コメントを元にコメント整理とメソッド切り出しをする
方針
- 英語コメントにしたことでコードとほぼ一致した箇所は、無価値コメントなので削除
- そうでないコメントは、メソッド切り出しをしてメソッド名として使って削除
差分
切り出されて生まれたメソッドはこう
+def sort_by_specified_key(kvs, key):
+ return sorted(kvs, key=lambda row: row[key])
+def filter_by_specified_key_and_value(kvs, key, value):
+ return filter(lambda row: row[key] == value, kvs)
+def extract_by_specified_key_and_first_one(kvs, key):
+ return kvs[0][key]
+def extract_by_specified_key(kvs, key):
+ return map(lambda row: row[key], kvs)
+def error():
+ raise Exception("no result")
本体部分はこうなった
-def write_first_one_or_empty(rows, sort_key, filter_key, filter_value, write_key):
+def write_first_one_or_empty(rows, sort_key, filter_key, filter_value, extraction_key):
- # as kvs
kvs = as_kvs(rows)
- # sort by specified key
- _sorted = sorted(kvs, key=lambda row: row[sort_key])
+ _sorted = sort_by_specified_key(kvs, sort_key)
- # filter by specified key and value
- _filtered = filter(lambda row: row[filter_key] == filter_value, _sorted)
+ _filtered = filter_by_specified_key_and_value(_sorted, filter_key, filter_value)
if 0 < len(_filtered):
- # extract by specified key and first one
- _mapped_one = map(lambda row: row[write_key], _filtered)[0]
- # write
- f.write([_mapped_one])
+ extracted_one = extract_by_specified_key_and_first_one(_filtered, extraction_key)
+ f.write([extracted_one])
else:
- # write empty
f.write([])
-def write_all_or_empty(rows, filter_key, filter_value, write_key):
+def write_all_or_empty(rows, filter_key, filter_value, extraction_key):
- # as kvs
kvs = as_kvs(rows)
- # filter by specified key and value
- _filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
+ _filtered = filter_by_specified_key_and_value(kvs, filter_key, filter_value)
if _filtered:
- # extract by specified key
- _mapped = map(lambda row: row[write_key], _filtered)
- # write
- f.write(_mapped)
+ extracted = extract_by_specified_key(_filtered, extraction_key)
+ f.write(extracted)
else:
- # write empty
f.write([])
-def write_all_or_error(rows, filter_key, filter_value, write_key):
+def write_all_or_error(rows, filter_key, filter_value, extraction_key):
- # as kvs
kvs = as_kvs(rows)
- # filter by specified key and value
- _filtered = filter(lambda row: row[filter_key] == filter_value, kvs)
+ _filtered = filter_by_specified_key_and_value(kvs, filter_key, filter_value)
if _filtered:
- # extract by specified key
- _mapped = map(lambda row: row[write_key], _filtered)
- # write
- f.write(_mapped)
+ extracted = extract_by_specified_key(_filtered, extraction_key)
+ f.write(extracted)
else:
- # error
- raise Exception("no result")
+ error()
次の問題
-
filter_keyとfilter_valueを==することしかできないので、例えばage < 20の様なフィルターが不可能 - リストへのインデックスアクセスが頻出している
step-3 少し局所リファクタリングをする
差分
インデックスアクセスでは少し不親切なのでheadとtailを用意した
+def head(xs):
+ return xs[0]
+def tail(xs):
+ return xs[1:]
利用箇所
def as_kvs(rows):
- keys = rows[0]
- value_rows = rows[1:]
+ keys = head(rows)
+ value_rows = tail(rows)
def extract_by_specified_key_and_first_one(kvs, key):
- return kvs[0][key]
+ return head(kvs)[key]
filter_by_specified_key_and_valueはもう少し融通が利くようにkey, valueではなくpredicateを受け取るように変更
-def filter_by_specified_key_and_value(kvs, key, value):
- return filter(lambda row: row[key] == value, kvs)
+def filter_by_predicate(kvs, predicate):
+ return filter(predicate, kvs)
そうするとfilter_by_predicateはfilter以上の事を何もしていないので破棄してしまう
-def filter_by_predicate(kvs, predicate):
- return filter(predicate, kvs)
それからheadを作ったのでextract_by_specified_key_and_first_oneもわざわざ用意してあげなくても良いかもしれない
小さく分けたメソッドの名前がちゃんとしていれば、組み合わせて使っても処理が不明瞭にはなりづらい
-def extract_by_specified_key_and_first_one(kvs, key):
- return head(kvs)[key]
ここまでを反映した本体部分はこう
-def write_first_one_or_empty(rows, sort_key, filter_key, filter_value, extraction_key):
+def write_first_one_or_empty(rows, sort_key, predicate, extraction_key):
kvs = as_kvs(rows)
_sorted = sort_by_specified_key(kvs, sort_key)
- _filtered = filter_by_specified_key_and_value(_sorted, filter_key, filter_value)
+ _filtered = filter(predicate, _sorted)
if 0 < len(_filtered):
- extracted_one = extract_by_specified_key_and_first_one(_filtered, extraction_key)
+ extracted_one = head(_filtered)[extraction_key]
f.write([extracted_one])
else:
f.write([])
-def write_all_or_empty(rows, filter_key, filter_value, extraction_key):
+def write_all_or_empty(rows, predicate, extraction_key):
kvs = as_kvs(rows)
- _filtered = filter_by_specified_key_and_value(kvs, filter_key, filter_value)
+ _filtered = filter(predicate, kvs)
if _filtered:
extracted = extract_by_specified_key(_filtered, extraction_key)
f.write(extracted)
else:
f.write([])
-def write_all_or_error(rows, filter_key, filter_value, extraction_key):
+def write_all_or_error(rows, predicate, extraction_key):
kvs = as_kvs(rows)
- _filtered = filter_by_specified_key_and_value(kvs, filter_key, filter_value)
+ _filtered = filter(predicate, kvs)
if _filtered:
extracted = extract_by_specified_key(_filtered, extraction_key)
f.write(extracted)
else:
error()
次の問題
- 書き込むか異常ハンドルかの部分にやや重複コードがある
- 処理が似ているので共通化が出来そう
step-4 コードの見た目が似ている箇所を共通化する（悪い方針）
方針
- ３つある本体メソッドの下２つ、その中の
filterからf.writeまでのコードが完全に同じなので、そこを切り出す
差分
+def filter_and_extract_and_write_if_not_empty(kvs, predicate, extraction_key):
+ _filtered = filter(predicate, kvs)
+
+ if _filtered:
+ extracted = extract_by_specified_key(_filtered, extraction_key)
+ f.write(extracted)
切り出されたので本体の行数が減った
def write_all_or_empty(rows, predicate, extraction_key):
kvs = as_kvs(rows)
- _filtered = filter(predicate, kvs)
+ filter_and_extract_and_write_if_not_empty(kvs, predicate, extraction_key)
- if _filtered:
- extracted = extract_by_specified_key(_filtered, extraction_key)
- f.write(extracted)
- else:
+ if not kvs:
f.write([])
def write_all_or_error(rows, predicate, extraction_key):
kvs = as_kvs(rows)
- _filtered = filter(predicate, kvs)
+ filter_and_extract_and_write_if_not_empty(kvs, predicate, extraction_key)
- if _filtered:
- extracted = extract_by_specified_key(_filtered, extraction_key)
- f.write(extracted)
- else:
+ if not kvs:
error()
この方針の問題
- kvs は異常ハンドルに必要なので共通化出来ない
- 切り出された
filter_and_extract_and_write_if_not_emptyが流用しづらい- 今後４つめの本体メソッドが出来たとして、これにぴったり合致する処理が必要になるか？
- メソッド名が長い
- 単語が多く、
andが使われている
- 単語が多く、
-
andを持つ場合は、やってくれることが多い- やってくれることが多いと、理解が難しくなる
- やってくれることが多いと、再利用時に用途が合致しづらくなる
-
ifの方だけ切り出したので、else部は本体側に残った- 切り出された処理の中を全部読まないと使い方も異常ハンドルをどうしたら良いかも分からない
何が悪かったのか
- 本来の処理の流れはこうだった
- データ構造変換
- 抽出
- 加工
- 書き出し or 異常ハンドル
- それをこうまとめてしまった
- データ構造変換
- 抽出・加工・書き出し
- or 異常ハンドル
- コードが似ているという理由で切り出しをしてはならず、処理の流れの単位で切り出さないとうまくいかない
step-4 処理の流れを整理する（良い方針）
先述の悪い方針は切り戻してやりなおす
方針
- 似たコードではなく、リストを書き込む箇所の流れに注目する
差分
本体メソッドの２つ目に注目する
今回はリストの中身がある場合と空リストの場合で処理を明示的に分ける必要はない
基本的にリストを渡す先の処理が同じなら、リストの長さを意識する必要は無い（してはいけない）
ここは中身があるか空リストかを知らぬままf.writeに渡してしまう方が良い
def write_all_or_empty(rows, predicate, extraction_key):
kvs = as_kvs(rows)
_filtered = filter(predicate, kvs)
- if _filtered:
- extracted = extract_by_specified_key(_filtered, extraction_key)
- f.write(extracted)
- else:
- f.write([])
+ extracted = extract_by_specified_key(_filtered, extraction_key)
+ f.write(extracted)
次に本体メソッドの１つ目にも注目する
こちらは空リストでは無い場合に先頭要素を取り出して加工してまたリスト化している
この
- 先頭要素を取り出す
- 単一要素の加工
- 再リスト化
という流れを
- 長さが最大 1 のリストで保持する
-
mapで全件（１つだが）加工する
という流れに発想を変えてみる
まずはリストから合致する先頭要素を最大長さ 1 で得るメソッドを用意する
+def find_first(kvs, predicate):
+ for kv in kvs:
+ if predicate(kv):
+ return [kv]
+ else:
+ return []
そうしてしまえば先ほどの本体メソッドの２つ目と同様に中身があるかどうかを意識する必要はなくなる
しかもリスト変換なのでheadとkeyアクセスではなくextract_by_specified_keyも使える
def write_first_one_or_empty(rows, sort_key, predicate, extraction_key):
kvs = as_kvs(rows)
_sorted = sort_by_specified_key(kvs, sort_key)
+ first = find_first(_sorted, predicate)
- _filtered = filter(predicate, _sorted)
-
- if 0 < len(_filtered):
- extracted_one = head(extract_by_specified_key(_filtered, extraction_key))
- f.write([extracted_one])
- else:
- f.write([])
+ extracted = extract_by_specified_key(first, extraction_key)
+ f.write(extracted)
本体メソッド１と２のif節もelse節も「リスト長を意識しない書き出し処理」という粒度に収まった
悪い例ではifとelseは一連の書き出し処理の仲間だったのに、ifの方だけを切り出してしまっていたことが悪手だった
次の問題
- アクセス修飾子を気にしていない
-
csvというモジュールだがただのリスト操作の処理がある
step-5 切り出したメソッドの整理をする
方針
- 切り出したそれぞれが
publicかprivateかを整理する
差分
-
headやtailは今後別のモジュールでも使いたくなりそうだからpublic- もっと考えると、こいつらはリスト操作しかしていないので
l.pyを作ってcsv.pyから追い出す - （
list.pyでも良かったけど、標準のlist()と名前が競合するので適当にずらした）
- もっと考えると、こいつらはリスト操作しかしていないので
l.py
+# -*- coding: utf-8 -*-
+
+
+def head(xs):
+ return xs[0]
+
+
+def tail(xs):
+ return xs[1:]
+
+
+def find_first(xs, predicate):
+ for x in xs:
+ if predicate(x):
+ return [x]
+ else:
+ return []
- よく見ると
kvsを操作してソートしたり抽出したりもただの辞書操作なので、d.pyを作ってcsv.pyから追い出す- その際に変数名も
kvsから抽象的な適当な名前に変える
- その際に変数名も
d.py
+# -*- coding: utf-8 -*-
+
+
+def extract_by_specified_key(xs, key):
+ return map(lambda x: x[key], xs)
+
+
+def sort_by_specified_key(xs, key):
+ return sorted(xs, key=lambda x: x[key])
- 残った処理は
csv.pyのためだけにあるメソッドなのでprivate化する
-def as_kvs(rows):
+def __as_kvs(rows):
-def error():
+def __error():
なぜ一見便利な as_kvs を切り出さないか
kvsはcsv.pyの期待するデータ構造を内部で処理しやすくするためだけに変換して用意した一時的なデータ構造に過ぎない
kvsは実際にはdictだが、csv.pyの公開している引数にも戻り値にもdict型は現れていない
つまりモジュールの提供する機能としてはどこにもdict型は存在しないので、公開するべきではない
加えてas_kvsの引数の型はlistだが、header + [body]構成であることや全てのカラムが一致している事等の制約があるため、l.pyやd.pyよりはcsv.pyの非公開処理とする方が望ましいと考えた
次の問題
-
privateメソッドが何カ所から使われているか分かりづらい- （はずだったんだけど、案外他モジュールに出て行ってしまった...）
step-6 一度しか使われていない private メソッドをリファクタリングする（ここは賛否両論かも）
やや大きなモジュールになると、沢山のprivateがそれぞれどこからどんな風に使われているかが極めて把握しづらくなることがある
そうするとprivateメソッドのリファクタリングがしづらかったり影響範囲が分かりづらかったりして結構大変な目に遭うことがある
沢山 private があるけどそれぞれ１度しか使われていない様な例
public 1 -> private 1 -> private 2 -> private 3
public a -> private a -> private b
public x -> private x
これは実質３つのpublicが存在すると捉えた方が全体像の把握がはるかに容易だったりする
privateも割と安心してリファクタリング出来たりする
１度しか使われない private と色んな箇所に依存している private が混じっている例
public 1 -> private 1
|
V
public a -> private a -> private b
^
|
public x -> private x -> private y
private 1やprivate xはある程度簡単に変更出来るが、private bを軽い気持ちで直すと全てのpublicに影響する
方針
１箇所からしか使われないprivateを、メソッドの中に定義してみる
これは僕があるとき勝手にやってみた方法でちゃんとした出展や議論はないけど、割と良手だと思って使い捨てコードやソロプロジェクトでは多用している
差分
__errorは一箇所でしか使っていないので、defの中にdefで用意してみた
ついでにそれに伴い、どうせ外からは見えなくなったので目障りな__を削除した
def write_all_or_error(rows, predicate, extraction_key):
+ def write_or_error(kvs):
+ if kvs:
+ f.write(kvs)
+ else:
+ raise Exception("no result")
+
kvs = __as_kvs(rows)
_filtered = filter(predicate, kvs)
extracted = d.extract_by_specified_key(_filtered, extraction_key)
- if extracted:
- f.write(extracted)
- else:
- __error()
+ write_or_error(extracted)
__as_kvsは当然何カ所からも使われているので変更しない
最後の問題
- テストを書いてない
- このままだと file-io が頻出するのでテストが書きづらい
- file-io ならテストは不可能ではないが、標準出力やメール送信の様な処理だと本当にテスト出来なかったりする
step-7 変換と書き出しを分離してテストを書く
方針
- 処理の流れを大きく捉えると「変換」と「出力」に分けられる
- このうちしっかり動作確認をしたいのは「変換」の方
- 大抵の場合「出力」はテストしづらい
なのでモジュールの責任範囲を「変換」だけにして、テストを実装する
差分
「出力」はせずにreturnして終わることとする
それに伴いメソッド名もwrite_からextract_に変更した
-def write_first_one_or_empty(rows, sort_key, predicate, extraction_key):
+def extract_first_one_or_empty(rows, sort_key, predicate, extraction_key):
kvs = __as_kvs(rows)
_sorted = d.sort_by_specified_key(kvs, sort_key)
first = l.find_first(_sorted, predicate)
- extracted = d.extract_by_specified_key(first, extraction_key)
- f.write(extracted)
+ return d.extract_by_specified_key(first, extraction_key)
-def write_all_or_empty(rows, predicate, extraction_key):
+def extract_all_or_empty(rows, predicate, extraction_key):
kvs = __as_kvs(rows)
_filtered = filter(predicate, kvs)
- extracted = d.extract_by_specified_key(_filtered, extraction_key)
- f.write(extracted)
+ return d.extract_by_specified_key(_filtered, extraction_key)
-def write_all_or_error(rows, predicate, extraction_key):
- def write_or_error(kvs):
- if kvs:
- f.write(kvs)
+def extract_all_or_error(rows, predicate, extraction_key):
+ def it_or_error(xs):
+ if xs:
+ return xs
else:
raise Exception("no result")
kvs = __as_kvs(rows)
_filtered = filter(predicate, kvs)
extracted = d.extract_by_specified_key(_filtered, extraction_key)
- write_or_error(extracted)
+ return it_or_error(extracted)
結果importが消えるので、csv.pyは file-io の処理はしなくなったことが確認できた
-from private import f
テストはこんなイメージ
csv_test.py
# -*- coding: utf-8 -*-
import csv
assert csv.extract_first_one_or_empty(
[['status', 'code'], ['dead', '001'], ['active', '003'], ['active', '002']],
'code', lambda kvs: kvs['status'] == 'active', 'code'
) == ['002']
assert csv.extract_first_one_or_empty(
[['status', 'code'], ['dead', '001']],
'code', lambda kvs: kvs['status'] == 'active', 'code'
) == []
おしまい
完成形
csv.py
# -*- coding: utf-8 -*-
import l
import d
def __as_kvs(rows):
keys = l.head(rows)
value_rows = l.tail(rows)
return [dict(zip(keys, value_row)) for value_row in value_rows]
def extract_first_one_or_empty(rows, sort_key, predicate, extraction_key):
kvs = __as_kvs(rows)
_sorted = d.sort_by_specified_key(kvs, sort_key)
first = l.find_first(_sorted, predicate)
return d.extract_by_specified_key(first, extraction_key)
def extract_all_or_empty(rows, predicate, extraction_key):
kvs = __as_kvs(rows)
_filtered = filter(predicate, kvs)
return d.extract_by_specified_key(_filtered, extraction_key)
def extract_all_or_error(rows, predicate, extraction_key):
def it_or_error(xs):
if xs:
return xs
else:
raise Exception("no result")
kvs = __as_kvs(rows)
_filtered = filter(predicate, kvs)
extracted = d.extract_by_specified_key(_filtered, extraction_key)
return it_or_error(extracted)
初版と比べて
- コメントは一切なくなったが、メソッド名をちゃんと付けたので情報量は減っていない
-
ifやforの様な所謂システム処理がなく、全部メソッド呼び出しで済んでいる- これにより英文のような感覚で流れを把握することが出来る
- （内部
defがある場合はそこを読み飛ばして本文開始点から読む）
- 今後も役に立ちそうな
l.pyとd.pyが生まれた - ただの「変換」になり戻りがシンプルな
list型になったので、呼び元はこの後さらなる加工を行ったりすることが出来る - 戻りで得られたリストは別の変換が続けられるだけでなく、ログに出したりレポート作成に組み込んだりも自由に出来る
- テストが書ける / ある
- 手間はかかったが、実は行数が減っている
- これは双方から空行を消してさらに初版の方からコメントを消しても、もともとコメントのない完成品の方が行数が少ない
締め
要点
要点と言い切るほど端的に言えてない...
価値のないコメントは書かない
- コードをただ日本語化している様なコメント
-
return result # 結果を返却の様なヤツ、案外良くある
-
- 慣れると要らないし、そもそも要らない
- おとなしくコードを読もう
- 同じ事をコードとコメントで書くと言うことは、修正箇所を倍増させている
- そしてコメントはどう書いても実行に影響しないので、正しく直される保証がない
- 大抵の場合「手段」が書かれている
じゃあ逆に書くべきコメントとは？
- なぜ
-
# この処理は〜〜の仕様のためにあります、とか -
# 〜〜を考慮してこういう実装にしました、とか -
# 〜〜は〜〜によりあり得ないので例外とします、とか
-
- 仕様書のパスや言語やフレームワークのバグ issue の url 、とか
- その他基本的にあとで誰かがコードを読むだけでは理解できない付加情報をコメントする
- 大抵の場合「目的」が書かれている
そうは言ってもコメントがないと辛い、と思った場合
- コードが悪くてそう思う場合は、今回の例のようにコメントではなく
private化して名前をちゃんと付ける- コメントは間違っても動くけど、コードは間違えられない
- 再利用も出来る
- スキル不足で単純にコードが読めなかったり日本語がないと不安な場合は、脱コメントを訓練だと思って頑張って力量を上げよう
- 厳しいようだけど、そのレベルにチーム活動のレベルを下げることは出来ない
- よほど異様に難しいとか魔術とかは別だけど、大抵の業務システムのコーディングなんてそんなに難しいことはしていない
悪そうな private メソッド
- メソッド名に
andとか入ってるとかなり疑わしい- やることが多いメソッドはそれだけ理解しづらい
- やることが多いほど使える状況が特化していくので再利用しづらい
- 実装を読まないと使い方が分からない
- これとこれを引数で渡すと、こうしてああしてくれるのね？うんうん、って中の奥まで読む事、案外ない？
- 読まないと使えないなら、実質的には行数は減っていない
良い private メソッド
- 末端の
privateメソッドは、名前がシンプルでやることも１つである方が良い-
len()とかhead()とか.split()とか - もちろん
privateを集約する facade のprivateはその限りではないけど
-
- 中を読まなくても使える
- いちいち
head()の中身とか読もうとしないよね
- いちいち
private と public のメソッドの扱いの違い
- 単にアクセス修飾子としてではなく、モジュールとして公開したいかで考える
- そのメソッド単品で役に立てないような処理は公開してはいけない
- たくさんの小さい
publicを用意する方が、後々どんどん楽になる - 逆に整備されていないモジュールを増やしてしまうほど、後々どんどん辛くなる
何をテストするか
- 「入力」や「出力」ではなくて、「変換」に狙いを定めて入念にテストする
-
publicな公開されているメソッドはちゃんとテストする- それにより安心して組み合わせられる
コードの見た目が似ているという理由で共通化しない
- 先に処理の流れを整理して、やりたいことを抽象的に整理する
- そのやりたいことが似ている箇所を共通化する
- 本来やりたいことが違うのにたまたま一部だけ処理（手段）が同じ様なことがあるが、それを安易に切り出さない
-
filterをfind_firstにした箇所と最後までfilterだった箇所 -
if write else emptyとif write else errorのif部
-
def in def
- コメントをメソッド化して生まれた
privateメソッドが一箇所からしか呼ばれていない場合のテクニック - 有効なスコープが狭いほど理解の難易度が下がり影響範囲が狭くなる
- 多分大抵の言語で出来る
scala は素直に出来る
def upperJoin(xs: List[String], sep: String): String = {
def upper(xs: List[String]): List[String] = xs.map(_.toUpperCase)
def join(xs: List[String]): String = xs.mkString(sep)
join(upper(xs))
}
upperJoin(List("hello", "world"), " ") // HELLO WORLD
java でもFunction<T, R>とかを使えば出来なくはない
public static String upperJoin(List<String> xs, String sep) {
Function<List<String>, List<String>> upper = _xs -> _xs.stream().map(String::toUpperCase).collect(toList());
Function<List<String>, String> join = _xs -> _xs.stream().collect(joining(sep));
return join.apply(upper.apply(xs));
}
upperJoin(asList("hello", "world"), " "); // HELLO WORLD
haskell も出来る
というか発想が haskell の関数を値で定義する感覚から来ている
upperJoin xs sep = (join . upper) xs
where
upper = map (map toUpper)
join = intercalate sep
upperJoin ["hello", "world"] " " -- HELLO WORLD
なので java の例は haskell に近いし、python でこう書くことも出来る
（PEP 8 ではlambdaに名前を付けて束縛するのはコーディング規範違反だけど、ネストが嫌なので一人だとこっちの方が多い）
def upper_join(xs, sep):
upper = lambda xs: [x.upper() for x in xs]
join = lambda xs: sep.join(xs)
return join(upper(xs))
upper_join(['hello', 'world'], ' ') # HELLO WORLD
反省
- 教科書的にはテストがあるところからリファクタリングするんだけど、そこをサボった
- 初版に「出力」を入れたかったのでテストがない状態から始めた
- まーこれくらいの規模なら間違っても気づけると思うし...
- そもそも完成後にさっとテスト書くくらいの方が僕は好き
-
kvsはdictのlistだったので、安易にd.pyに出したのはちょっと違った...- ま、そもそも例は雰囲気のためにしか存在しないので、いーけど
おしまい
やってみたかっただけなので、満足した
どうでも良いけど、赤地と緑地に黒い字ってのを見てるといっつも「スイカみたいだな〜」って思う