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


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

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

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

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

## 意味がないコメントは書かない
コードからすぐわかるようなことは書かないようにしましょう。

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

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

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

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

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

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

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

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

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

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

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

```py
# 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` チェックが必要なこともわかります。

```py
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](https://marketplace.visualstudio.com/items?itemName=ms-python.vscode-pylance)

<img width="588.6" alt="image.png (13.1 kB)" src="https://img.esa.io/uploads/production/attachments/5809/2021/12/21/19598/48e55b78-7992-4657-bdb4-30192de1efeb.png">

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

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

## dictではなく構造体で返す

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

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

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

dictを返却するメソッド

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

↓

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

```py
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](https://qiita.com/Ted-HM/items/7dde25dcffae4cdc7923)

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

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

↓

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

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

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

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

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

```python
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` よりは、かなりマシな実装です。

```python
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を利用することで、変数の変更を全く行わない実装です。

```python
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を使い、ループカウンタ自分で管理しなければならなくなります。

```python
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` は、`数字のみの要素` を無視する分岐を追加するだけです。
※ ただし分岐が増えるとつらくなってきます。

```python
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がないのが悔やまれます

```python
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. スコープは狭く

## クラスや関数を外部環境に依存させない
グローバル変数など、スコープの広い変数に依存すると、関数が参照透過でなくなりテスト不能なります。

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

↓

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

## 早期リターンを活用する
以下のコードは  `長い処理` を読んでいる間、ずっと `else` の分岐が気になってしまいます。

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

↓

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

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

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

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

    def _method2(self):
        print(self.message)
    
    # そのほか、多数の self.messagesを使わない処理
```
↓
`self.message` をローカル変数化することで、インスタンス変数を削除することができました。
このクラスはステートを持たないので、いつだれが実行しても同じ結果を返します。テストしやすいです。

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

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


# 5. 変化に強いコードを書くために
## 共通化の罠
似ている処理を共通化したくなるのがプログラマの性ですが、
目的や変更理由が異なるコードのロジックを <font color="red">似ているから</font> という理由で一つにまとめるべきではありません。

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

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

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

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

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

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

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

こうなります。

```python
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` 側は利用しないにもかかわらず、考慮しなければなりません。
<font color="red">※ 基本的に「手続き」は共通化しないほうがいい場合が多いです。</font>
<font color="red">※ 経験則ですが、引数に処理を分けるフラグが入ったら分割の合図です。 </font>


これはほんの一例です。このあたりの話はこちらの資料がとても参考になります。
[オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling](https://speakerdeck.com/sonatard/coheision-coupling)

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

```python
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` の入力をイテレータにするとテスト可能になります。

```python
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両方の入力に対応できるようになります。

```python
# ファイルから読み込む
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` にまで波及します。


```python
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` 側に修正は不要です。

```python
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](https://www.amazon.co.jp/dp/B07FSBHS2V/)
- [リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック](https://www.amazon.co.jp/dp/4873115655/)
- [オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling](https://speakerdeck.com/sonatard/coheision-coupling)
- [世界一わかりやすいClean Architecture | YouTube](https://www.youtube.com/watch?v=pbCRHAM5NG0)

# 参考

- [Clean Architecture　達人に学ぶソフトウェアの構造と設計 | Amazon](https://www.amazon.co.jp/dp/B07FSBHS2V/)
- [リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック | Amazon](https://www.amazon.co.jp/dp/4873115655/)
- [オブジェクト指向のその前に-凝集度と結合度/Coheision-Coupling](https://speakerdeck.com/sonatard/coheision-coupling)
- [プログラミングでよく使う英単語のまとめ【随時更新】 | Qiita](https://qiita.com/Ted-HM/items/7dde25dcffae4cdc7923)