Zenn
🐍

Pythonのコードをレビューするときに見ているもの

2020/09/21に公開
Python
tech

はじめに

業務で数年間 Python ばかり書き続けています。Python には本当にいろいろな書き方があります。単純に言語仕様としても拡張がなされ続けていますし、そもそも Python のユーザー層が多岐にわたります。わたしが出会っただけでも、 Web エンジニア、インフラエンジニア、データサイエンティスト、機械学習エンジニア、研究者、ちょっとした自動化をする趣味プログラマなどなどです。そして書く人、書く目的や文脈などによって、何が良いコードなのかは全く変わるものです。

ある程度いろいろなコードを自分でも書いて、人が書いてきたの読んできました。そのせいか、レビューをしようと PR の diff を開くと、「これは怪しい」と思う Python コードか、それとも「書きなれているんだろうなこの人」と思うかでわりと二極化されます。要するに、良くないコードがあるとなんか匂うわけです。

ここでは僕が匂いを感じる（感じた）コードについて書こうと思います。どれも初歩的な内容だと思います。順番にとくに意味はないですが、一応簡単なものから書いているつもりです。

それから、わたしは Web のバックエンドエンジニアなので、それ以外の文脈でのレビューはしません。自分しか使わないスクリプトがどう書かれていようが特に気にしませんし、急いでいるときや一回ぽっきりの実行コードで ugly なコードを受け入れたりもします。ただ、ここに書くことはどんな文脈でも守ったほうが better だと思います。

長いブロック

まずは見た目です。見た目で読みづらいコードは匂いとか関係なく、diff を見るとすぐにわかるものです。

Python の言語仕様としてブロックをインデントで表現するというのがあります。賛否あるところで、実際に僕自身インデントのせいで生じたバグに遭遇したこともあります。

それを Python の言語仕様のせいだと糾弾するのは簡単ですが、そもそもブロックをインデントで表現しようが {} で表現しようが、ノートパソコンのディスプレイに収まらないほど長いブロックを書くのをまずやめるべきです。それだけでインデントのせいによるバグは激減すると思います。

ブロックが長くなり始めたらヘルパー関数を用意して処理を分割しましょう。多くの場合、ヘルパー関数は汎用的な処理ではないので、そのモジュールやクラスの protected メソッドとして作るだけで十分です（protected メソッドは言語仕様として存在するわけではなく、名前を _ から始めた場合はそれとみなすコミュニティの慣習です）。

深いネスト

Python に限った話ではありませんが、ネストが深いコードはやめましょう。状況によっては、簡単なテクニックでネストを浅くできます。早期リターン、早期 continue を意識するだけです。

具体例を出すと、こんな形の for - if - for - if なんてコードをまれに見ることがあります（下記の条件にとくに意味はないです）。フィルタリングの条件が複雑だと、こんなコードを書くことも起こりえます。

results = []
for a in l:
    if a % 2 == 0:
        for b in m:
            if a != b:
                results.append(b)

これはこんな風にすると、ネストが減ります。やっていることは、条件を反転させて continue しているだけです。機械的にできるので、↑みたいなコードを書いたときはじっと見直して考え直すとよいかと。

results = []
for a in l:
    if a % 2 != 0:
        continue
    for b in m:
        if a == b:
            continue
        results.append(b)

否定の使い方

まず、人類には否定は難しいです。原則として肯定を使うことを考えましょう。0 以上ではないより0未満である（あるいは負の数である）と表現したほうがよいです。

not (a >= 0)
# ではなく
a < 0

とはいえ否定の論理式をやみくもに避けるのも難しいです。避けられないときは、 not の数やかかる範囲を最小限にするとよいと思います。

not a is None
# ではなく
a is not None

not a in l
# ではなく
a not in l

not a and not b
# ではなく
not (a or b)

Django の ORM でも、肯定で書くほうがわかりやすいはずです。 QuerySet から要素を抽出するときは、基本的には .filter() を使いましょう。.filter(field__in=targets) と .exclude(fields__in=targets) では後者のほうが理解するのに時間がかかるはずです（もちろん、否定のほうがわかりやすい時、パフォーマンス上否定にせざるを得ないときもあります）。

他にも != と and を連発している条件式とか、どうあがいても難しい条件式も実際あります。 ただ、否定は難しさの温床、という意識があるだけで書き方が変わるかもです。

try 節をよりよく使う

例として HTTP の POST リクエストを送る処理を挙げます。クライアントはともかく、リクエスト先（サーバー）のエラーが起こりえるので、こうしたコードはエラー処理が必須です。

よくあるのが、下記のようなコードです。

try:
    foo = {'foo': 12}
    res = client.post(URL, json=foo)
    print(res.status)
except:
    res = None

かなり単純化しているので「こんなの書かないやろ」と思うようなコードになっていますが、これの派生形ならよく見ます（現物は見せられませんが）。

このコードには下記の突っ込みどころがあります。

  • foo を代入した時点では絶対にエラーが起きないのになぜ try の中にあるのか
  • client.post メソッドのうちどんなエラーを想定しているのかわからない
  • エラーを握りつぶしているので res が None のままこのコードは実行されるがそれで問題ないのかわからない

そのため、原則として下記を意識するといいです。

  • try 節の中はエラーを起こす可能性のある行のみ入れる。無意味に肥大化させない。
  • except には想定しているエラーを書く。原則として全てのエラーをキャッチしない
  • res に None を入れるのではなく例外を返してユーザーのエラー処理にゆだねる（後述）
foo = {'foo': 12}
try:
    res = client.post(URL, json=foo)
except ClientError as e:  # 呼び出し元がおかしい場合
    raise e
except:  # サーバー側がおかしい場合
    raise ServerError(...)
else:
    print(res.status)

むやみに None を返さない

try 節の話に関連しますが、なんでもエラーが出たら None を返す関数を書く人が居ます。

def reshape_array(a):
    try:
        a = np.reshape(12, -1)
    except:
        return None
    return a

None を返すのは、基本的に「ユーザーに例外処理を押し付ける」行為です。なんか知らんがうまくいかんかった、理由は知らん、デバッグはお前がやれ、というコードです。

ここで例外を返せば、例外処理ができていない実装だとプログラムは止まりますが、一般論としてプログラムは想定していない例外をつぶしたまま動き続けるよりは止まったほうがいいです。何気ない None ですが、これが原因で原因の特定が難しいバグを埋め込む、なんてことは本当によくあります。

reshape に失敗したとき（おそらく raise されるのは ValueError でしょう）、どう処理するべきかは関数の使い手の問題です。関数はユーザーに選択権を残すべきです。None はその選択権を奪います。

キーワード引数に mutable オブジェクトが入っている

Python の言語仕様で、やり玉にあげられるもののひとつです。

def foo_increment(d={'foo': 0}):
    d['foo'] += 1
    return d

print(dict_init())  #  => {'foo': 1}
print(dict_init())  #  => {'foo': 2}

dict_init のキーワード引数の値は「関数の定義時」に評価されます（言い換えれば関数の呼び出しごとに初期化されるわけではありません）。なのでこの d は（多くの場合、意図せず）状態を持ってしまいます。

Python の組み込み型で mutable なものはリスト、辞書、集合の3つです。この3つをキーワード引数にとりたい場合は、Noneを初期値として、関数内で初期化するのが better です。

def foo_increment(d=None):
    if d is None:
        d = {'foo': 0}
    d['foo'] += 1
    return d

まとめ

書き終わって見てみると、そんなに Python に限った話をしていないように思います。最後のキーワード引数の話は Python の話ですが。

こういうことを書くと Linter ではじけば、と言われる気がするのですが、歴史的経緯があると Linter は入れづらいんですよね。

それはそれとして、普段自分はコードレビューするときはブロックの長さとネストの深さ try 節だけ見て、だいたいを判断します。そこで NG ならドメインのコードについても重大な誤りがあるかもしれない、と疑ってかかる試金石のような形です。誰でも指摘できるような欠点が残っているということは、深く考えた PR ではない可能性が高いのではないか、とみています。とはいえ、とくに新機能実装とかだとどうしても難しいコードが表出することはあるので、ある程度は仕方ないのですが。

さて、いろいろと書きましたが正直なところ、僕が書くよりも Effective Python を読めばよいと思います。もっと詳しく知りたい人はぜひ、 Effective Python 第二版を買ってください。おすすめです（筆者はこの書籍の関係者ではありません）。

GitHubで編集を提案

Discussion