ソースコードレビューで最近指摘したこと

プログラムのソースコードレビューで私がよく指摘することをまとめようと思い、とりあえず直近のレビューフィードバックの内容を振り返ってみるメモです。

まず、Pythonのインデントについて。Pythonのインデント指向の文法は好きではないが、読みやすいようにインデントを活用していきたい。

トリプルクォート(triple-quoted strings)を使って文字列を定義するときにインデントが崩れてしまうことがよくある。しかしインデントが崩れているとメソッドやクラスの定義範囲が読み取りずらくなる。

改善前

def foo(...):
    sql = """
SELECT a, b, c
FROM bar
"""
    ...

変更後

import textwrap

def foo(...):
    sql = textwrap.dedent(
        """
        SELECT a, b, c
        FROM bar
        """
    )
    ...

文字列の中でもインデントできるように textwrap.dedent を使う。（textwrap.dedentを使わなくてもSQLなら動くかもしれないが、デバッグでSQLの中身を見るときのために）

インデント関連では似たものとして、

改善前

aaa.foo1(argumant1 = "barbar", argumant2 = "bazbaz", argumant3 = "hogehoge")
aaa.foo2(
    argumant1 = "barbar", argumant2 = "bazbaz", argumant3 = "hogehoge", argumant4 = "fugafuga", argumant5 = "piyopiyo"
)

似たような関数呼び出しがあったときに、たまたま1行の長さが少しだけ長いほうだけを改行を入れているケースがある。見比べやすいように1行が短いほうも同じ折り返しの仕方にする。

改善後

aaa.foo1(
    argumant1 = "barbar", argumant2 = "bazbaz", argumant3 = "hogehoge"
)
aaa.foo2(
    argumant1 = "barbar", argumant2 = "bazbaz", argumant3 = "hogehoge", argumant4 = "fugafuga", argumant5 = "piyopiyo"
)

長い文字列を構築するときは

改善前

def foo(...):
    sql = textwrap.dedent(
        """
        SELECT ...
        FROM ...
        """
    )
    if 条件1:
        sql += " WHERE baz = 2"
    if 条件2:
        if 条件1:
            sql += " WHERE qux = 3"
        else:
            sql += " AND qux = 3"

SQLを文字列で組み立てるのがいいのかどうかはここでは置いておいて、文字列で組み立てざるを得ないときにどうするか。これはPythonに限った話ではない。

こういう取ってつけたようなコードは今後の保守の過程で加速度的に複雑なコードになってしまう。条件式が増えたときにコードが表現したい内容以上に複雑になってしまう。なのでこういうふうにする。

改善後

def foo(...):
    sql = textwrap.dedent(
        """
        SELECT ...
        FROM ...
        """
    )
    where_conds = []
    if 条件1:
        where_conds.append("baz = 2")
    if 条件2:
        where_conds.append(" qux = 3")
    if where_conds:
        sql += " WHERE " + " AND ".join(where_conds)

条件に応じてSQLのWHERE句に比較式の追加が必要だという仕様をそのまま書く。条件に応じたWHERE句と、SQLの文字列を組み立てる処理を別々に書いて、記述箇所の焦点を絞る。表現したいことをストレートに表現できるようにする。