はじめに
新人のころ、先輩からコードレビューを受ける際、よく耳にしたのが、
「うーん、細かいがおれならこう書くよ。まあ、間違ってはないけど」
当初は、そのありがたみがよくわからず、しぶしぶ直していましたが、
今になって、先輩の「細かいけど伝えたい」気持ちがわかったような。
先輩の指摘事項(Python例)
シングルクォートか、ダブルクォートか
- 自分
print('I\'ll be back')
- 先輩
print("I'll be back")
エスケープ不要な書き方に。
メソッド名は長くてもよい
- 自分
def check_file(file_name: str):
# check if a file exists
- 先輩
def check_if_file_exists(file_name: str):
メソッド名から機能を読み取れるように(コメントより効果的)。
同じ処理を何回も書かない
- 自分
def func1():
if cond1:
do_something1()
else:
do_something2()
def func2():
if cond1:
do_something1()
else:
do_something2()
- 先輩
def common_process():
if cond1:
do_something1()
else:
do_something2()
def func1():
common_process()
def func2():
common_process()
同じ処理はメソッド新設、可読性とメンテナンス性向上。
値が指定範囲に含まれるかチェック
- 自分
# (0, 100)区間に含まれる場合
if val > 0 and val < 100:
do_something1()
# [0, 100]区間に含まれない場合
if val < 0 or val > 100:
do_something2()
- 先輩
# (0, 100)区間に含まれる場合
if 0 < val < 100:
do_something1()
# [0, 100]区間に含まれない場合
if val < 0 or 100 < val:
do_something2()
不等号の向きをそろえることで、右向きのx軸上を動く点(値val)をイメージ。
余分な変数を作らない
- 自分
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
- 先輩
def get_basic_info(student: Student) -> dict:
basic_info = {
"name": student.name,
"age": student.age,
"birth": student.birth,
}
return basic_info
変数の無駄遣い、プログラムが冗長に。
辞書の最後にカンマを残してもOK(キーの追加や順序変更時便利)
文字列のNone・空文字列チェックを同時に
- 自分
if val is None or val == "":
do_something()
- 先輩
if not val:
do_something()
1つのnotで用が足りる。
まっさきに異常系を処理しreturnさせる
- 自分
if cond1:
if cond2:
# 正常系の処理
... ...
... ...
... ...
else:
# 異常系の処理(エラーログ出力など)
else:
# 異常系の処理(エラーログ出力など)
- 先輩
if not cond1:
# 異常系の処理(エラーログ出力など)
return
if not cond2:
# 異常系の処理(エラーログ出力など)
return
# 正常系の処理
... ...
... ...
... ...
深いインデントが解消され、ロジックがシンプルに。
bool型処理で余計な分岐を作らない
- 自分
def is_correct_answer() -> bool:
if cond1:
return True
else:
return False
- 先輩
def is_correct_answer() -> bool:
return cond1
やりたかったことが1行に。
長い条件は変数新設
- 自分
if long_long_long_long_long_cond1 and long_long_long_long_long_cond2:
do_something()
- 先輩
cond1 = long_long_long_long_long_cond1
cond2 = long_long_long_long_long_cond2
if cond1 and cond2:
do_something()
長い条件を羅列すると可読性低下。
変数名cond1、cond2に意味合いを持たせたらなおよい。
リストをばらけるなら
- 自分
age_list = [20, 21, 22]
age1 = age_list[0]
age2 = age_list[1]
age3 = age_list[2]
- 先輩
age_list = [20, 21, 22]
age1, age2, age3 = age_list
タプルもアンパックできますね。
辞書から指定キーの値を取得
- 自分
if "name" in student_dict:
name = student_dict["name"]
else:
name = ""
- 先輩
name = student_dict.get("name", "")
キーの存在チェックまで1行で解決。
ファイルはコンテキストマネージャー(with文)
- 自分
try:
f = open("test.txt", "r")
except Exception as e:
... ...
else:
data = f.read()
f.close()
- 先輩
with open("test.txt", "r") as f:
data = f.read()
with文がcloseまでやってくれる。
一時的な作業ディレクトリがほしい
- 自分
import os
import shutil
temp_dir = "temporary_dir"
os.makedirs(temp_dir, exist_ok=True) # ディレクトリを作成
do_something()
shutil.rmtree(temp_dir) # ディレクトリを削除
- 先輩
import tempfile
with tempfile.TemporaryDirectory() as temp_dir:
do_something()
ディレクトリの削除までやってくれる。
親クラスのタイプまでチェックしたい
class Parent:
pass
class Child(Parent):
pass
item = Child()
- 自分
if type(item) is Child or type(item) is Parent:
do_something()
- 先輩
if isinstance(item, Parent):
do_something()
type()は、インスタンスの直のクラスのみチェック、
isinstance()は、継承元も含めてチェックできる。
おわりに
細かすぎて伝わらないかもしれないコードレビューでした。
もっとよい書き方も存在すると思いますので、ご参考まで。