oshiro の日記

I love good unittest, low level programing and nice team

メソッド名は抽象的でよいと思う

プログラミング

最近見たコードで「ん？これは。。。」というコードを見かけたので、そこら辺にスポットした話を書こうと思います。

まずは何も言わずに下記のコードを見てください。

class CsvGenerator():
    def generate_csv():
        xxx

    def generate_csv_from_json():
        yyy

    def generate_csv_with_header():
        zzz

賢明な方は即座にお気づきかと思いますが、3点ほど気になった箇所がありますよね。

メソッド名の中で CSV が重複してる
with_header はオプションなので引数で外部から制御したい
public メソッドに切りすぎ
メソッド名が具体的過ぎて拡張できない

4個になってしまった、、、

まぁ違和感としては3箇所なんですが、具体化すると4点ありました。

最後2点に関してはタイトル通りなんですが、具体的過ぎて結局実装者もその使用者も「やりたいことがこのメソッドではできないじゃないか、きーっ」ってなってしまうので、問題点としては抽象度がなさすぎるということですかね。

なので、再度まとめると

メソッド名の中で CSV が重複してる
with_header はオプションなので引数で外部から制御したい
抽象度がなさすぎる

ということが問題かなと思います。

では1点ずつ見ていきましょう。

メソッド名の中で CSV が重複してる

やっぱり気になるのはこれでしょう。 CsvGenerator なので, 最終的に出力されるものが CSV であることは当然ですよね。

いや、おれは k8s を使うからどうしても yaml を出力したいんだよぉ！！っていう方はポリモーフィズム使ってもう一段階抽象度の高いクラスを作って OOP しようよって話でしょうか。

SubDbMigtartor で execute すればサブ DB を migrate するだろうことは共通認識だろうし、FileReader なら read すればファイルの中身を返してくれるだろうことは炊飯器のスイッチを押せばご飯が炊けるくらい当たり前のことですよね。

基本的にはクラスを作成する際にはひとつのことだけうまくやるように作ることが原則です。今回の例でいえば CsvGenerator は csv ファイルを生成することだけできれば構わないので、メソッド名としては generate でよいのではないでしょうか。

とはいえ、おれは一つの炊飯器でもっちりも早炊きもできないと嫌なんだよ！！！という人も少なくはないでしょう。詳しくは次のトピックで書きますが、とはいえ炊飯器はご飯を炊くことに特化した機械であり、期待する能力は炊飯であるということですね。

最近の炊飯器はパンも作れたりしますけどね。。。

with_header はオプションなので引数で外部から制御したい

要はご飯を炊くオプションとして早炊きやらもっちりだきやらを指定したいということですね。

先の書き方ではどうでしょうか。ヘッダーのある CSV と無い CSV の2パターンを出力したいということです。

2 パターン程度であれば外部からの制御でなくともよいかも知れませんが、これが4つ5つとなってくるとどうでしょうか。

def with_header()

def with_index()

def without_blank_line()

なんて考えたら実装するのも大変ですが、使用者からしたらたまったもんではないですね。「ヘッダーは欲しいけど空白行は詰めてある CSV が欲しい」なんて可能性もあります。

なのでこのような制御の利かないメソッドの一番の問題は条件に対して組み合わせができないことです。

外観がボタンで埋め尽くされた炊飯器なんて使いたくないですよね。。。

ではどうするか。

一般的なコードであればこのようになります。

def generate(filename. header = false, index = false. blank_line = false):
    xxx
    return csv_text

def _add_header():

def _remove_blank_line():

def _add_index():

コマンドラインから直接呼ばれるようなものであれば、argparse モジュールのようなもので外部からオプションとして渡せるようにするのがよいと思います。

もちろん抽象化することで直感的な使用はできなくなってしまいますが、この CsvGenerator の使用者の自由度は格段に上がったと思いませんか？

ライブラリを作成するにあたっては OSS や社内 PJ 、個人開発を問わずに非開発者が使いやすい構成になっているかということは非常に重要です。最悪内部実装が間に合っていなくてもインターフェースの整理は優先的にやった方がよいと思います。具体的な内部実装は private メソッドにやってもらいましょう。

抽象度がなさすぎる

さて、最後にタイトルを回収させてください。実装レベルでの修正は先の２トピックでほぼ行いましたので、どちらかというと抽象的な話になります。

クラス名やメソッド名についてどうとらえてますか？

クラス名やメソッド名は役割であり、責務を表すものだと思っています。なのでクラス名やメソッド名が上手くつけられない場合は責任が広がりすぎていることが多いです。人間はあまり抽象化が得意な生き物ではありませんので、良く抽象化された名前を考えるよりも名前を付けやすいところまで責任を分割してしまう方が楽です。

開発者目線だけでなく、使用者の目線で考えることも抽象化には有効です。例えば内部処理の例ですが

def exist_file(path):
    return path が存在するか判定

def is_csv_file(path):
    return path が存在するか判定

raise Exception if exist_file or is_csv_file

という場合と

def shold_throw_exception(path):
    return __exist_file() or __is_csv_file()

raise Exeption if shold_throw_exception()

ではどちらが可読性がありそうでしょうか？後者では shold_throw_exception という関数に例外を投げるための条件がまとめられているため、ファイルが無い場合と CSV じゃなかった場合に例外を投げることが明示できます。例外を投げる条件が増えても対応しやすいですね。

多くの場合他人が書いたコードをについて注意深く参照するのはクラス名とメソッド名、その引数と戻り値くらいじゃないですか？

CsvGenerator.generate('foo.xlsx') を実行した場合、内部で yaml に変換されてようが Json を経由していようとしったこっちゃないわけですね。 generate が csv の文字列を返して、generate_file で csv ファイルのパスとかファイルオブジェクトを返すくらいの柔軟性は期待の範囲内でしょう。

もちろん動作速度を高速なものを探す場合などは別の話ですが。。。

まとめ

クラス名が明示的であれば public メソッドは抽象的な方がよい

クラス名やメソッド名は役割であり、責務を表すもの

という上記2点だけお伝えできればと思います。

今後より内容の濃いものを分かりやすく簡潔に伝えられるように精進しますので、応援よろしくお願い致します。内容に関して修正点や意見がございましたらコメントいただけますととても嬉しいです