Pythonで理解するコーディングアンチパターン

条件分岐編

if文のネスト

コードが長いとリファクタリングしにくく複雑化していくため、必要の無いネストは廃止しましょう。

bad

age = int(input())
if age >= 0:
    if age < 20:
        print("You're a minor")

good

age = int(input())
if 0 <= age < 20:
    print("You're a minor")

複雑な条件式に説明変数や関数抽出を行わない

if文ネストと同様に説明変数や関数抽出を行わないコードは複雑化しやすいため、条件を落とし込んで、条件分岐の可読性を高めましょう。

bad

threshold = 60
score_limit = 100
math_score = int(input())
english_score = int(input())
science_score = int(input())
if threshold <= math_score <= score_limit and threshold <= english_score <= score_limit and threshold <= science_score <= score_limit:
    print("You have passed all the subjects")
else:
    print("You have　not passed all the subjects")

good

def is_all_subjects_passed(*args):
    threshold = 60
    score_limit = 100

    for score in args:
        if not threshold <= score <= score_limit:
            return False
    return True


math_score = int(input())
english_score = int(input())
science_score = int(input())
if is_all_subjects_passed(math_score, english_score, science_score):
    print("You have passed all the subjects")
else:
    print("You have not passed all the subjects")

if-else文の否定文を先に記述

否定文から先に書くと可読性がやや下がるので、なるべく肯定文を先に書くようにしましょう。

bad

savings = int(input())
if not savings > 0:
    print("You have no savings")
else:
    print("Your savings is {} yen".format(savings))

good

savings = int(input())
if savings > 0:
    print("Your savings is {} yen".format(savings))
else:
    print("You have no savings")

変数編

マジックナンバーを使う

後々コードを見返す際、何の数なのか分からなくなるため、説明変数や定数に落とし込むようにしましょう。

bad

# ['6', '7', '8']だけでは、何を示しているのか分からない
if input() in ['6', '7', '8']:
    print("It's summer now")
else:
    print("It's not summer now")

good

month = input()
summer_seasons = ['6', '7', '8']
if month in summer_seasons:
    print("It's summer now")
else:
    print("It's not summer now")

定義した場所と使用する場所が離れている

変数と使用する場所が離れていると可読性が下がることと、スコープ把握の観点から近くで定義するようにしましょう。

bad

age = 15
# height,genderは二つ目以降のifで使われるため、ここで宣言する必要がない
height = 175
gender = 'man' 

# 定数であるAVERAGE_HEIGHT_OF_MANについても、ここで宣言する必要がない
ADULT_AGE = 20
AVERAGE_HEIGHT_OF_MAN = 170

if 0 <= age <= ADULT_AGE:
    print("You're a minor")
else:
    print("You're not a minor")

if height >= AVERAGE_HEIGHT_OF_MAN:
    print("You're taller than average height")
else:
    print("You're shorter than average height")

if gender == "man":
    print("You are a man")
elif gender == "woman":
    print("You are a woman")
else:
    print("Gender isn't just male and female, is it?")

good

age = 15
ADULT_AGE = 20
if 0 <= age <= ADULT_AGE:
    print("You're a minor")
else:
    print("You're not a minor")

height = 175
AVERAGE_HEIGHT_OF_MAN = 170
if height >= AVERAGE_HEIGHT_OF_MAN:
    print("You're taller than average height")
else:
    print("You're shorter than average height")

gender = 'man'
if gender == "man":
    print("You are a man")
elif gender == "woman":
    print("You are a woman")
else:
    print("Gender isn't just male and female, is it?")

特定の関数でしか使わない変数をグローバルで定義

無駄にグローバル変数を設けてしまうと、意図せずにその変数にアクセスしてしまい、予期せぬタイミングで値を変えてしまう恐れがあります。なるべくスコープは小さく保ちましょう。

bad

class Person:

    def __init__(self, name, age):
        self.name = name
        self.age = age

    def introduce_yourself(self):
        print("=========================")
        print("My name is {}".format(self.name))
        print("I'm {} years old now".format(self.age))
        print("=========================")


person = Person("kite", 15)
person.introduce_yourself()

# personクラスの変数を書き換えられてしまう。
person.name = "michel"
person.age = 35
person.introduce_yourself()

=========================
My name is kite
I'm 15 years old now
=========================
=========================
My name is michel
I'm 35 years old now
=========================

good

class Person:

    def __init__(self, name, age):
        self.__name = name
        self.__age = age

    def introduce_yourself(self):
        print("=========================")
        print("My name is {}".format(self.__name))
        print("I'm {} years old now".format(self.__age))
        print("=========================")


person = Person("kite", 15)
person.introduce_yourself()
person.__name = "michel"
person.introduce_yourself()

=========================
My name is kite
I'm 15 years old now
=========================
=========================
My name is kite
I'm 15 years old now
=========================

発音しにくい命名

発音しにくいということは、理解しにくいということに関わってきます。その変数や関数が何のために存在するのかを示す適切な名前に変更しましょう。

bad

# Dictionary for managing personal informationの略であるが、読みにくく分かりにくい
dfmpi = {"name":"taylor", "age":20}
print("My name is {}".format(dfmpi["name"]))
print("I'm {} years old".format(dfmpi["age"]))

good

# Dictionary for managing personal information
personal_info = {"name":"taylor", "age":20}
print("My name is {}".format(personal_info["name"]))
print("I'm {} years old".format(personal_info["age"]))

検索しにくい命名

ファイル内で変数を検索する場面は多くあります。その際に同じプレフィックスの変数が存在すると、検索などで同じ様な変数の候補が出されて煩雑となるため、なるべく固有の変数としましょう。

bad

import datetime

dt = datetime.datetime.now()
# 検索欄でvariableの候補が最低でも3つ出ることとなり、煩雑となる
variable_for_current_second = dt.second
variable_for_current_minute = dt.minute
variable_for_current_hour = dt.hour
print("{}時{}分{}秒".format(variable_for_current_hour, variable_for_current_minute,
                        variable_for_current_second))

good

import datetime

dt = datetime.datetime.now()
sec = dt.second
min = dt.minute
hour = dt.hour
print("{}時{}分{}秒".format(hour, min, sec))

関数編

同じ処理を関数にせず複数箇所に記述

同じ処理を繰り返すことは、コードを肥やす原因となります。関数に落とし込んで再利用しましょう。

bad

spring_season = [3, 4, 5]
summer_season = [6, 7, 8]
fall_season = [9, 10, 11]
winter_season = [12, 1, 2]
season = None
month = int(input())

if month in spring_season:
    season = "spring"
elif month in summer_season:
    season = "summer"
elif month in fall_season:
    season = "fall"
elif month in winter_season:
    season = "winter"
print("Current season is {}".format(season))

next_month = int(input())
if next_month in spring_season:
    season = "spring"
elif next_month in summer_season:
    season = "summer"
elif next_month in fall_season:
    season = "fall"
elif next_month in winter_season:
    season = "winter"
print("Next month's season is {}".format(season))

good

def get_current_season(month):
    spring_season = [3, 4, 5]
    summer_season = [6, 7, 8]
    fall_season = [9, 10, 11]
    winter_season = [12, 1, 2]
    season = None

    if month in spring_season:
        season = "spring"
    elif month in summer_season:
        season = "summer"
    elif month in fall_season:
        season = "fall"
    elif month in winter_season:
        season = "winter"
    return season


month = int(input())
print("Current season is {}".format(get_current_season(month)))
print("Next month's season is {}".format(get_current_season(month + 1)))

関数名から何をするか分からない

関数名が理解できないコードは、何でも屋になる恐れがあります。一つの関数には一つのことを行わせるためにも、適切な関数名をつけましょう。

bad

class Dog:

    def __init__(self, name, age):
        self.__name = name
        self.__age = age

    # get,setだけでは、何に対してgetし、何に対しsetするか分からない
    def get(self):
        return self.__name

    def set(self, name):
        self.__name = name

good

class Dog:

    def __init__(self, name, age):
        self.__name = name
        self.__age = age

    def get_name(self):
        return self.__name

    def set_name(self, name):
        self.__name = name

引数にbooleanを渡す

引数にbooleanを渡すということは、その値がtrueかfalseかを判定するメソッドでなければならなくなります。即ち、boolean以外の引数がある場合、関数が複数のことを行うことにつながってしまいます。こうならないよう、なるべくbooleanは引数に渡さないようにしましょう

bad

def calculate_annual_income(　is_there_bonus, saraly, monthly_benefit):
    all_months = 12
    total_saraly = salary * all_months
    yearly_benefit = monthly_benefit * all_months
    annual_income = (total_saraly + yearly_benefit) * 0.8
    
    # この時点で年収計算しているのに、booleanがあることでボーナスがあるか否かの判定もしなければならない。
    if is_there_bonus:
        bonus_magification = 2.5
        annual_income += salary * bonus_magification
    return annual_income

good

def calculate_annual_income(bonus, saraly, monthly_benefit):
    all_months = 12
    total_saraly = salary * all_months
    yearly_benefit = monthly_benefit * all_months
    annual_income = (total_saraly + yearly_benefit) * 0.8 + bonus
    return annual_income

関数が複数のことをしている

複数の事をさせると、容易に関数が肥えてしまいます。リファクタリングのことも考えて短くしましょう。

bad

import math

# ざっくりと面積を計算するメソッドのため、多くのことを許容してしまっている
def calculate_area(bottom, upper_bottom, height, radius):
    assert bottom * upper_bottom * height * radius > 0
    triangle_area = bottom * height / 2
    trapezoid = (bottom + upper_bottom) * height / 2
    circle = radius**2 * math.pi
    return triangle_area, quadrilateral, trapezoid, circle

good

import math


def calculate_traiangle_area(bottom, height):
    return bottom * height / 2


def calculate_trapezoid_area(bottom, upper_bottom, height):
    return (bottom + upper_bottom) * height / 2


def calculate_circle(radius):
    return radius**2 * math.pi