細かいけど伝えたかった先輩のコードレビュー

### はじめに

新人のころ、先輩からコードレビューを受ける際、よく耳にしたのが、
「うーん、細かいがおれならこう書くよ。まあ、間違ってはないけど」

当初は、そのありがたみがよくわからず、しぶしぶ直していましたが、
今になって、先輩の「細かいけど伝えたい」気持ちがわかったような。

### 先輩の指摘事項(Python例)

#### シングルクォートか、ダブルクォートか

- 自分
```Python
print('I\'ll be back')
```

- 先輩
```Python
print("I'll be back")
```

エスケープ不要な書き方に。

#### メソッド名は長くてもよい

- 自分
```Python
def check_file(file_name: str):
    # check if a file exists
```

- 先輩
```Python
def check_if_file_exists(file_name: str):
```

メソッド名から機能を読み取れるように(コメントより効果的)。

#### 同じ処理を何回も書かない

- 自分
```Python
def func1():
    if cond1:
        do_something1()
    else:
        do_something2()

def func2():
    if cond1:
        do_something1()
    else:
        do_something2()
```

- 先輩
```Python
def common_process():
    if cond1:
        do_something1()
    else:
        do_something2()

def func1():
    common_process()

def func2():
    common_process()
```

同じ処理はメソッド新設、可読性とメンテナンス性向上。

#### 値が指定範囲に含まれるかチェック

- 自分
```Python
# (0, 100)区間に含まれる場合
if val > 0 and val < 100:
    do_something1()
```

```Python
# [0, 100]区間に含まれない場合
if val < 0 or val > 100:
    do_something2()
```

- 先輩
```Python
# (0, 100)区間に含まれる場合
if 0 < val < 100:
    do_something1()
```

```Python
# [0, 100]区間に含まれない場合
if val < 0 or 100 < val:
    do_something2()
```

不等号の向きをそろえることで、右向きのx軸上を動く点(値`val`)をイメージ。

#### 余分な変数を作らない

- 自分
```Python
def get_basic_info(student: Student) -> dict:
    name = student.name
    age = student.age
    birth = student.birth
    basic_info = {
        "name": name,
        "age": age,
        "birth": birth
    }
    return basic_info
```

- 先輩
```Python
def get_basic_info(student: Student) -> dict:
    basic_info = {
        "name": student.name,
        "age": student.age,
        "birth": student.birth,
    }
    return basic_info
```

変数の無駄遣い、プログラムが冗長に。
辞書の最後にカンマを残してもOK(キーの追加や順序変更時便利)

#### 文字列のNone・空文字列チェックを同時に

- 自分
```Python
if val is None or val == "":
    do_something()
```

- 先輩
```Python
if not val:
    do_something()
```

1つの`not`で用が足りる。

#### まっさきに異常系を処理しreturnさせる

- 自分
```Python
if cond1:
    if cond2:
        # 正常系の処理
        ... ...
        ... ...
        ... ...
    else:
        # 異常系の処理(エラーログ出力など)
else:
    # 異常系の処理(エラーログ出力など)
```

- 先輩
```Python
if not cond1:
    # 異常系の処理(エラーログ出力など)
    return
if not cond2:
    # 異常系の処理(エラーログ出力など)
    return
# 正常系の処理
    ... ...
    ... ...
    ... ...
```

深いインデントが解消され、ロジックがシンプルに。

#### bool型処理で余計な分岐を作らない

- 自分
```Python
def is_correct_answer() -> bool:
    if cond1:
        return True
    else:
        return False
```

- 先輩
```Python
def is_correct_answer() -> bool:
    return cond1
```

やりたかったことが1行に。

#### 長い条件は変数新設

- 自分
```Python
if long_long_long_long_long_cond1 and long_long_long_long_long_cond2:
    do_something()
```

- 先輩
```Python
cond1 = long_long_long_long_long_cond1
cond2 = long_long_long_long_long_cond2
if cond1 and cond2:
    do_something()
```

長い条件を羅列すると可読性低下。
変数名`cond1`、`cond2`に意味合いを持たせたらなおよい。

#### リストをばらけるなら

- 自分
```Python
age_list = [20, 21, 22]
age1 = age_list[0]
age2 = age_list[1]
age3 = age_list[2]
```

- 先輩
```Python
age_list = [20, 21, 22]
age1, age2, age3 = age_list
```

タプルもアンパックできますね。

#### 辞書から指定キーの値を取得

- 自分
```Python
if "name" in student_dict:
    name = student_dict["name"]
else:
    name = ""
```

- 先輩
```Python
name = student_dict.get("name", "")
```

キーの存在チェックまで1行で解決。

#### ファイルはコンテキストマネージャー(with文)

- 自分

```Python
try:
    f = open("test.txt", "r")
except Exception as e:
    ... ...
else:
    data = f.read()
    f.close()
```

- 先輩
```Python
with open("test.txt", "r") as f:
    data = f.read()
```

with文がcloseまでやってくれる。

#### 一時的な作業ディレクトリがほしい

- 自分
```Python
import os
import shutil

temp_dir = "temporary_dir"
os.makedirs(temp_dir, exist_ok=True)  # ディレクトリを作成
do_something()
shutil.rmtree(temp_dir)  # ディレクトリを削除
```

- 先輩
```Python
import tempfile

with tempfile.TemporaryDirectory() as temp_dir:
    do_something()
```

ディレクトリの削除までやってくれる。

#### 親クラスのタイプまでチェックしたい

```Python
class Parent:
    pass

class Child(Parent):
    pass

item = Child()
```

- 自分
```Python
if type(item) is Child or type(item) is Parent:
    do_something()
```

- 先輩
```Python
if isinstance(item, Parent):
    do_something()
```

`type()`は、インスタンスの直のクラスのみチェック、
`isinstance()`は、継承元も含めてチェックできる。

### おわりに

細かすぎて伝わらないかもしれないコードレビューでした。
もっとよい書き方も存在すると思いますので、ご参考まで。