チーム開発のお作法 ~ 半年後の自分のために

なんだか大層なタイトルですが、自分がチーム開発をする中で、最低限これは守ったほうがいいんじゃないかなってことをまとめてみます。
CleanArchitectureやリーダブルコードにはもっとたくさんの素晴らしい設計やノウハウがありますが、ひとまず自分の経験則からくる最低限です。

最近Pythonを使うことが多いのでサンプルコードはPythonで書こうと思います。

1. コメント

ドキュメントコメントを書く

引数や戻り値の型と意味が明確になると可読性が格段に上がります。
※静的型付け言語やタイプヒントを利用している場合、型は書かなくてもOKです。

def indexof(haystack, needle):
    """検索文字列が最初に現れる位置を返す
    Aargs:
        haystack (str): 検索対象の文字列
        needle (str): 検索文字列
    Returns:
        int: 検索文字列が見つかった位置
    """
    # ... 処理 ...
    return index

意味がないコメントは書かない

コードからすぐわかるようなことは書かないようにしましょう。

# リクエストオブジェクトを初期化
request = Request()

# セッションにcontent_idをセット
request.session["content_id"] = content_id

半年後の自分に向けた心の声を書く

理解しにくい箇所を簡潔に説明しましょう

input = ["1", "2", "a", "b", 5]
# 数字を抜き出してintにキャストする
list(map(lambda e: int(e), filter(lambda e: str(e).isdigit(), input)))
# [1, 2, 5]

定数やマジックナンバーの理由を説明しましょう

# awsの文字数制限が32文字なので8文字以上に設定すべきではない
STAGE_NAME_LINIT = 8

# 1ページに1000件以上表示するとブラウザが固まる
fetch_item(limit=1000)

心の声をコメントに残しましょう。
※ 人の目を気にして、きれいにまとめようとか、英語で書こうとか思わないこと！

TODO: や FIXME: などの注釈を入れるとコメントの意図がわかりやすくなります。

• TODO: 今後改善すべきコード
• FIXME: 既知のバグがあるコード
• HACK:実装が汚いコード
• NOTE:留意してほしいポイント
• XXX: よくわからないけど、とにかく危ないコード

# TODO: 件数が多いので二分木探索を利用した実装にすべき。
item = liner_search(item_list, needle)

# FIXME: SQLインジェクションの危険性。bindを利用した実装に修正すべき。
sql = f"SELECT name, age, division　From user WHERE user={user_name}"
conn.execute(sql)

# HACK: デバッグ・テスト不能なコード。Hogeクラスにsortメソッドを実装すべき。
subprocess.call(["sort", src_file, ">", dst_file])

# NOTE: コネクションはワーカープロセスごとに一つだけ作る。シングルトンで実装してもいいかも。
conn = Connection()

# XXX: hogehogeが定義されていないため、動かないはずだが現環境では動いている。なぜだろう。。。
unknown(hogehoge=None)

2. 型のサポートを利用する

タイプヒントを利用する

型を明示することで、スクリプト言語特有の引数や戻り値の型の間違いを防止できます。 ( 数値を期待している引数に文字列を渡したり )
この例では戻り値が Optional[] となっているので、戻り値の None チェックが必要なこともわかります。

from typing import Optional, List
def difficult_processing(x: int, y: int, z: int) -> Optional[List[int]]:
    # ... 複雑な処理 ...
    return result

vscodeなどのモダンなエディタを利用していれば、Pylanceなどのプラグインで静的解析や定義ジャンプが可能です。
Pylance | Visual Studio Marketplace

listではなくtupleで返す

複数の要素を返却したいときはlistではなくtupleを使いましょう。
listは要素がいくつ入っているのかわかりません。
tupleなら型ヒントと合わせて使うことで、要素数と型が明確になります。

def return_tuple() -> Tuple[str, int]: # 型ヒントで戻り値が明確になる
    return ("hoge", 1)

dictではなく構造体で返す

dictを返却すると、次の理由でデバッグやコードを追うのが非常に難しくなるのでやめましょう。

1. 実行するまで構造がわからない (読んで構造を理解できるのはインタプリタだけ！)
2. エディタの静的解析が効かない (コードを追っているときに行き止まりになります)

dict形式のレスポンスを返却したくなったら構造体を作りましょう。要素の型や構造が明確になります。

dictを返却するメソッド

class SampleClass:
    def return_dict(self):
        ret = {}
        # ... とっても複雑な処理 ...
        return ret # 何が入っているかわからない...

↓

構造体を返却するメソッド
※ Pythonで構造体を作るときは dataclass を利用するとよいです。 frozen=True を引数に取ることでイミュータブルにできます。

from dataclasses import dataclass

@dataclass(frozen=True)
class ResultObj:
    x: List[int]
    y: Optional[str]
    z: float

class SampleClass:
    def return_obj(self) -> ResultObj:
        # ... とっても複雑な処理 ...
        return ResultObj(x=x, y=y, z=z) # 中の構造が明確！

3. 変数

分かりやすい変数名を付ける

変数の共通認識を理解するとコードの可読性が上がります。

• get_xxx: 軽量アクセサ。中で重い処理を行っていないイメージ
• is_xxx has_xxx exists contains : boolean型が入っているイメージ
• head tail : headはlistの一番最初の要素、tailはそれ以降のすべての要素のイメージ
• などなど

こちらの記事にはよくお世話になっています。
プログラミングでよく使う英単語のまとめ【随時更新】 | Qiita

使うときに定義する

変数を関数の頭ですべて定義しているコードを見ますが、基本的には使うときに定義するほうが可読性が上がります。　
変数がどこで利用されるかを意識しながら読むのは大変だからです。

def fuga():
    result = None
    # ... 長い処理 ...
    result = Hoge(ret)

↓

def fuga():
    # ... 長い処理 ...
    result = Hoge(ret)

一度定義したら原則変更しない

変数を変更するということはステートを持つということです。
場所によって変数の中身が異なると可読性が大幅に損なわれます。
スクリプト言語は異なる型の値を代入できたりするので、よりカオスになりがちです。
特に引数に再代入するのは絶対に避けましょう。引数で渡された dict や list に対しての追加や削除もNGです。(sortなど参照の変更を前提としたものは除く)

例えば、テキストをスペースで分割して、それぞれの単語を正規化する関数を考えてみます。

入力: uild 1 High-Level 2 that 3 provide 4
出力: ['uild', '1', 'high-level', '2', 'that', '3', 'provide', '4']

split_term_1 は terms の要素を「変更」しています。(いわゆる変数を変更する実装)

def split_term_1(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    for i in range(0, len(terms)):
        terms[i] = terms[i].strip().lower() # termsの要素を書き換える
    return terms

split_term_2 は normalized_terms というlistを新しく作り、正規化した単語を追加していく実装です。
※ normalized_terms を変更していますが、既存の値を変更しない分 split_term_1 よりは、かなりマシな実装です。

def split_term_2(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    normalized_terms = [] # 変数名からtermを正規化したいことがわかる
    for term in terms:
        normalized_terms.append(term.strip().lower()) # listを更新しているが、それ用に作ったlistなのでまだよい
    return normalized_terms

split_term_3 はmapを利用することで、変数の変更を全く行わない実装です。

def split_term_3(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    normalized_terms = list(map(lambda e: e.strip().lower(), terms)) # 変数の更新を全く行っていない
    return normalized_terms

このレベルだと何が良くて何が悪いのかピンと来ないかもしれないかもしれませんが、次の機能追加を考えてみましょう。
変数の変更がいかに可読性を下げるかがよくわかります。

• 数字のみの要素 を除外する

split_term_1 はfor文では実装できません。whileを使い、ループカウンタ自分で管理しなければならなくなります。

def split_term_1(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    i = 0 # ステートがさらに増える
    while (i < len(terms)):
        if (re.match("^[0-9]+$", terms[i])):
            # 数字のみの要素は削除(ループカウンタは更新しない)
            del terms[i]
        else:
            # そのほかの要素は正規化
            terms[i] = terms[i].strip().lower()
            i += 1
    return terms

split_term_2 は、数字のみの要素 を無視する分岐を追加するだけです。
※ ただし分岐が増えるとつらくなってきます。

def split_term_2(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    normalized_terms = [] # 変数名の意味が少しずれる
    for term in terms:
        if (re.match("^[0-9]+$", term)): # 数字のみの要素を無視する分岐を追加するだけでいい
            continue
        normalized_terms.append(term.strip().lower())
    return normalized_terms

split_term_3 は、分岐を考慮する必要すらなく、数値を除外する処理を追加するだけでOKです。
※ PythonにScalaのようなStreamがないのが悔やまれます

def split_term_3(text: str) -> List[str]:
    """テキストをスペースで分割して、それぞれの単語に正規化処理を行う"""
    terms = text.split(" ")
    filterd_terms = filter(lambda e: not re.match("^[0-9]+$", e), terms) # この行を追加するだけ
    normalized_terms = map(lambda e: e.strip().lower(), filterd_terms)
    return list(normalized_terms)

4. スコープは狭く

クラスや関数を外部環境に依存させない

グローバル変数など、スコープの広い変数に依存すると、関数が参照透過でなくなりテスト不能なります。

def hoge():
    os.getenv("DATABASE_URL") # 関数内で取得せずに引数で渡すべき
    # ...処理...

↓

def hoge(database_url: str):
    # ...処理...

早期リターンを活用する

以下のコードは 長い処理 を読んでいる間、ずっと else の分岐が気になってしまいます。

def some_process(text: str):
    if (len(text) > 0):
        # 長い処理...
    else:
        return None

↓

返せるものは早めに返して、頭のメモリを開放しましょう。

def some_process(text: str):
    if (len(text) <= 0):
        return None
    # 長い処理

インスタンス変数は最小限に

インスタンス変数はいろいろなメソッドによって変更されるため、それ自体がステートとなります。
ステートは可読性やテスタビリティを下げる要因になります。
不必要なインスタンス変数はつくらないようにしましょう。

class LargeClass:
    message: str = ""
    
    def method1(self):
        # 処理...
        self.message = "hello"
        self._method2()

    def _method2(self):
        print(self.message)
    
    # そのほか、多数の self.messagesを使わない処理

↓
self.message をローカル変数化することで、インスタンス変数を削除することができました。
このクラスはステートを持たないので、いつだれが実行しても同じ結果を返します。テストしやすいです。

class LargeClass:
    def method1(self):
        # 処理...
        message = "hello"
        self._method2(message)

    @staticmethod
    def _method2(message: str):
        print(message)
    
    # そのほか、多数の self.messagesを使わない処理

5. 変化に強いコードを書くために

共通化の罠

似ている処理を共通化したくなるのがプログラマの性ですが、
目的や変更理由が異なるコードのロジックを 似ているから という理由で一つにまとめるべきではありません。

例えば、内部の処理が似ている some と other があるとします。これを some_other にまとめるのはよくありません。

def some():
    a()
    b()
    c()
    d()

def other():
    a()
    b()
    d()

↓

def some_other(is_some: bool):
    a()
    b()
    if (is_some):
        c()
    d()

このレベルではまだわからないかもしれませんが、 some の挙動だけ変更したいのに other を考慮する必要があるといった具合に、容易に修正できないコードになります。
※ some と other の目的や変更理由が同じなら、どちらか一方だけを変更したいという要件は生まれません

例えば、こんな要件が追加になったとします

• some だけ a()を一番最後に持っていくことになった
• other に flag が追加になって、 flag==False の場合に d() を e() にするパターンが生まれた

こうなります。

def some_other(is_some: bool, other_flag: bool):
    if (not is_some):
        a()
    b()
    if (is_some):
        c()
    if (is_some or other_flag) # some側もother_flagを考慮するかのような意味の分からないコードになる
        d()
    else:
        e()
    if (is_some):
        a()

ひどいですね、、、 other_flag に至っては some 側は利用しないにもかかわらず、考慮しなければなりません。
※ 基本的に「手続き」は共通化しないほうがいい場合が多いです。
※ 経験則ですが、引数に処理を分けるフラグが入ったら分割の合図です。

これはほんの一例です。このあたりの話はこちらの資料がとても参考になります。
オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling

外部リソースに依存しない

例えば、よく見かけるこんなコード。 filepath を引数で受け取って、ファイルを開いて処理します。
一見問題ないように見えますが、このコードはファイルという外部リソースに依存しているため、変更に弱く、テスト不能です。

def count_term(filepath: str, target: str) -> int:  # 引数にファイルパスをとっている
    """ファイル内のtargetの数を数える"""
    counter = 0
    with open(filepath, "r") as fh:
        for line in fh.readlines():
            terms = line.strip().split(" ")
            for term in terms:
                normalized = term.lower()
                if (normalized == target):
                    counter += 1
    return counter

↓
ファイルの読み出しイテレータを生成する関数を作り、count_term の入力をイテレータにするとテスト可能になります。

from typing import Iterable

def file_lines(filepath: str) -> Iterable[str]:
    """ファイルを1行ずつ読み出すイテレータを返す"""
    with open(filepath, "r") as fh:
        for line in fh.readlines():
            yield line.strip()

def count_term(lines: Iterable[str], target: str) -> int: # 引数にIterableをとるようになった(インターフェースへの依存になった)
    """テキスト内のtargetの数を数える"""
    counter = 0
    for line in lines:
        terms = line.strip().split(" ")
        for term in terms:
            normalized = term.strip(" .,'").lower()
            if (normalized == target):
                counter += 1
    return counter
    

このコードは、ファイルとlist両方の入力に対応できるようになります。

# ファイルから読み込む
reader = file_lines("sample.txt")
print(count_term(reader, "to"))

# listから読み込む
lines = [
    "The definition of fibs is a val not a method.",
    "The memoization of the Stream requires us to have somewhere to store the information and a val allows us to do that."
]
print(count_term(lines, "to"))

さらに素晴らしいところは、たとえ入力元がDBになっても、OSが変わっても count_term を編集する必要はないということです。

具象クラスに依存しない

次のコードは一見問題ないように見えますが、Entity (上位コンポーネント)と UseCase (下位コンポーネント)の関係性に問題があります。

• UseCase の Entity への依存範囲が明確ではないため、 Entity の修正が難しくなります。 ( method_3 を利用していないことが明確ではない)
• UseCase の内部で Entityを初期化しているので、 UseCaseの単体テストができません。
• UseCase が 具象クラス( Entity )に依存しているので、 Entity の修正が UseCase にまで波及します。

class Entity():
    """ビジネスロジックを実装しているクラス (上位のコンポーネント)
    """
    def method_1(self, a: str) -> str:
        return a

    def method_2(self, b: int) -> int:
        return b
    
    def method_3(self, c: bool) -> bool:
        return c

class UseCase:
    """ビジネスロジックを利用するクラス (下位のコンポーネント)
    """
    entity: Entity # 具象クラスに依存してしまっている
    
    def __init__(self):
        self.entity = Entity() # 内部で具象クラスのEntityを初期化してしまっている

    def main(self):
        # method_3は利用していないのに、明示的にそれが示されていない
        print(self.entity.method_1("hello"))
        print(self.entity.method_2(2))

if __name__ == "__main__":
    use_case = UseCase()
    use_case.main()

↓

• UseCase を具象クラス( Entity )ではなく、インターフェース( UseCaseInterface )に依存させることで、依存範囲を明確化し、 Entity , UserCase それぞれの独立性と変更容易性を高めます。
• UseCase は UseCaseInterface (インターフェース)を引数で受け取るため、モックを利用した単体テストが可能となります。
• Entity は UseCaseInterface の仕様を守ってさえいればいいので、修正が容易になります。
  たとえ Entity を丸ごとリプレースしても、 UseCaseInterface さえ実装されていれば UseCase 側に修正は不要です。

import abc
from dataclasses import dataclass
class UseCaseInterface(metaclass=abc.ABCMeta):
    """UseCaseクラスが必要とするインターフェース"""
    @abc.abstractmethod
    def method_1(self, a: str) -> str:
        raise NotImplementedError

    @abc.abstractclassmethod
    def method_2(self, b: int) -> int:
        raise NotImplementedError

class Entity(UseCaseInterface):
    """ビジネスロジックを実装しているクラス (上位のコンポーネント)
    """
    def method_1(self, a: str) -> str:
        return a

    def method_2(self, b: int) -> int:
        return b

    def method_3(self, c: bool) -> bool:
        return c

@dataclass(frozen=True)
class UseCase:
    """ビジネスロジックを利用するクラス (下位のコンポーネント)
    """
    # インターフェースに依存しているので、Entityの中身の処理を気にする必要がない
    # インターフェースからmethod_3を利用しないことが明確である
    # 外部から受け取るため、mockに置き換えてテストできる
    entity: UseCaseInterface  

    def main(self):
        print(self.entity.method_1("hello"))
        print(self.entity.method_2(2))
        
if __name__ == "__main__":
    entity = Entity()
    use_case = UseCase(entity=entity)
    use_case.main()

もっと知りたい方へ

今日話したことは、気を付けるべきことのほんの一部です。
体系的に理解したい方はこのあたりの本や資料を参考にされるといいかと思います。

• Clean Architecture　達人に学ぶソフトウェアの構造と設計 | Amazon
• リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック
• オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling
• 世界一わかりやすいClean Architecture | YouTube

参考

• Clean Architecture　達人に学ぶソフトウェアの構造と設計 | Amazon
• リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック | Amazon
• オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling
• プログラミングでよく使う英単語のまとめ【随時更新】 | Qiita