551
442

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 1 year has passed since last update.

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

Last updated at Posted at 2022-11-01

はじめに

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

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

先輩の指摘事項(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()

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

リストをばらけるなら

  • 自分
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()は、継承元も含めてチェックできる。

おわりに

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

551
442
16

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
551
442

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?