Python code smells 實(shí)例講解

很多時(shí)候一段代碼符合基本邏輯，能夠正常運(yùn)行，并不代表它是不“丑”的。代碼中可能會存在諸如可讀性差、結(jié)構(gòu)混亂、重復(fù)代碼太多、不夠健壯等問題。

示例代碼

"""
Very?advanced?Employee?management?system.
"""

from?dataclasses?import?dataclass
from?typing?import?List

#?The?fixed?number?of?vacation?days?that?can?be?paid?out.
FIXED_VACATION_DAYS_PAYOUT?=?5


@dataclass
class?Employee:
????"""Basic?representation?of?an?employee?at?the?company"""

????name:?str
????role:?str
????vacation_days:?int?=?25

????def?take_a_holiday(self,?payout:?bool)?->?None:
????????"""Let?the?employee?take?a?single?holiday,?or?pay?out?5?holidays."""
????????if?payout:
????????????if?self.vacation_days?????????????????raise?ValueError(
????????????????????f"You?don't?have?enough?holidays?left?over?for?a?payout.?\
????????????????????????????Remaining?holidays:?{self.vacation_days}"
????????????????)
????????????try:
????????????????self.vacation_days?-=?FIXED_VACATION_DAYS_PAYOUT
????????????????print(
????????????????????f"Paying?out?a?holiday.?Holidays?left:?{self.vacation_days}")
????????????except?Exception:
????????????????pass
????????else:
????????????if?self.vacation_days?????????????????raise?ValueError(
????????????????????"You?don't?have?any?holidays?left.?Now?back?to?work,?you!"
????????????????)
????????????self.vacation_days?-=?1
????????????print("Have?fun?on?your?holiday.?Don't?forget?to?check?your?emails!")


@dataclass
class?HourlyEmployee(Employee):
????"""?Employee?that's?paid?based?on?number?of?worked?hours."""

????hourly_rate:?float?=?50
????amount:?int?=?10


@dataclass
class?SalariedEmployee(Employee):
????"""Employee?that's?paid?based?on?a?fixed?monthly?salary."""

????monthly_salary:?float?=?5000


class?Company:
????"""Represents?a?company?with?employees."""

????def?__init__(self)?->?None:
????????self.employees:?List[Employee]?=?[]

????def?add_employee(self,?employee:?Employee)?->?None:
????????self.employees.append(employee)

????def?find_managers(self)?->?List[Employee]:
????????managers?=?[]
????????for?employee?in?self.employees:
????????????if?employee.role?==?"manager":
????????????????managers.append(employee)
????????return?managers

????def?find_vice_presidents(self)?->?List[Employee]:
????????vice_presidents?=?[]
????????for?employee?in?self.employees:
????????????if?employee.role?==?"president":
????????????????vice_presidents.append(employee)
????????return?vice_presidents

????def?find_interns(self)?->?List[Employee]:
????????interns?=?[]
????????for?employee?in?self.employees:
????????????if?employee.role?==?"intern":
????????????????interns.append(employee)
????????return?interns

????def?pay_employee(self,?employee:?Employee)?->?None:
????????if?isinstance(employee,?SalariedEmployee):
????????????print(
????????????????f"Paying?employee?{employee.name}?a?monthly?salary?of?${employee.monthly_salary}"
????????????)
????????elif?isinstance(employee,?HourlyEmployee):
????????????print(
????????????????f"Paying?employee?{employee.name}?a?hourly?rate?of?\
????????????????????????????${employee.hourly_rate}?for?{employee.amount}?hours."
????????????)


def?main()?->?None:
????company?=?Company()
????company.add_employee(SalariedEmployee(name="Louis",?role="manager"))
????company.add_employee(HourlyEmployee(name="Brenda",?role="president"))
????company.add_employee(HourlyEmployee(name="Tim",?role="intern"))
????print(company.find_vice_presidents())
????print(company.find_managers())
????print(company.find_interns())
????company.pay_employee(company.employees[0])
????company.employees[0].take_a_holiday(False)


if?__name__?==?'__main__':
????main()

上述代碼實(shí)現(xiàn)了一個(gè)簡單的“員工管理系統(tǒng)”。

• Employee 類代表公司里的員工，有姓名、角色、假期等屬性?？梢哉埣伲?code style="font-size: 14px;font-family: "Operator Mono", Consolas, Monaco, Menlo, monospace;word-break: break-all;overflow-wrap: break-word;padding: 2px 4px;border-radius: 4px;margin-right: 2px;margin-left: 2px;color: rgb(233, 105, 0);background: rgb(248, 248, 248);">take_a_holiday），或者單獨(dú)請一天，或者以 5 天為單位將假期兌換為報(bào)酬
• HourlyEmployee 和 MonthlyEmployee 分別代表以時(shí)薪或者月薪來計(jì)算工資的員工
• Company 類代表公司，可以招收員工（add_employee）、返回特定角色的員工列表（如 find_managers）、發(fā)放薪資等（pay_employee）

code smells

上面的代碼中存在著很多可以改進(jìn)的地方。

用 Enum 類型替代 str 作為員工的 role 屬性

上面的 Employee 類使用了 str 類型來存儲 role 屬性的值，比如用 "manager" 代表經(jīng)理，用 "intern" 代表實(shí)習(xí)生。

company.add_employee(SalariedEmployee(name="Louis",?role="manager"))
company.add_employee(HourlyEmployee(name="Brenda",?role="president"))
company.add_employee(HourlyEmployee(name="Tim",?role="intern"))

實(shí)際上 String 過于靈活，可以擁有任何含義，用來表示角色屬性時(shí)不具有足夠清晰的指向性。不同的拼寫規(guī)則和大小寫習(xí)慣都會導(dǎo)致出現(xiàn)錯(cuò)誤的指向，比如 "Manager" 和 "manager"，"vice-president" 和 "vice_president"?？梢允褂?Enum 替代 str。

from?enum?import?Enum,?auto


class?Role(Enum):
????"""Employee?roles."""
????PRESIDENT?=?auto()
????VICEPRESIDENT?=?auto()
????MANAGER?=?auto()
????LEAD?=?auto()
????WORKER?=?auto()
????INTERN?=?auto()

修改 Employee 類中 role 屬性的定義：

@dataclass
class?Employee:

????name:?str
????role:?Role
????vacation_days:?int?=?25

Company 類中 find_managers 等方法也做相應(yīng)的修改：

def?find_managers(self)?->?List[Employee]:
????managers?=?[]
????for?employee?in?self.employees:
????????if?employee.role?==?Role.MANAGER:
????????????managers.append(employee)
????return?managers

main 方法中使用新的 role 創(chuàng)建員工對象：

company.add_employee(SalariedEmployee(name="Louis",?role=Role.MANAGER))
company.add_employee(HourlyEmployee(name="Brenda",?role=Role.VICEPRESIDENT))
company.add_employee(HourlyEmployee(name="Tim",?role=Role.INTERN))

消除重復(fù)代碼

Company 類中有一個(gè)功能是返回特定角色的員工列表，即 find_managers、find_vice_presidents、find_interns 三個(gè)方法。

這三個(gè)方法實(shí)際上有著同樣的邏輯，卻分散在了三個(gè)不同的函數(shù)里。可以合并成一個(gè)方法來消除重復(fù)代碼。

def?find_employees(self,?role:?Role)?->?List[Employee]:
????"""Find?all?employees?with?a?particular?role."""
????employees?=?[]
????for?employee?in?self.employees:
????????if?employee.role?==?role:
????????????employees.append(employee)
????return?employees

同時(shí)將 main 函數(shù)中的 find_managers、find_vice_presidents、find_interns 都改為如下形式：

print(company.find_employees(Role.VICEPRESIDENT))
print(company.find_employees(Role.MANAGER))
print(company.find_employees(Role.INTERN))

盡量使用內(nèi)置函數(shù)

上面版本中的 find_employees 方法，包含了一個(gè) for 循環(huán)。實(shí)際上該部分邏輯可以使用 Python 內(nèi)置的列表推導(dǎo)來實(shí)現(xiàn)。

合理的使用 Python 內(nèi)置函數(shù)可以使代碼更短、更直觀，同時(shí)內(nèi)置函數(shù)針對很多場景在性能上也做了一定的優(yōu)化。

def?find_employees(self,?role:?Role)?->?List[Employee]:????
????"""Find?all?employees?with?a?particular?role."""????
????
????return?[employee?for?employee?in?self.employees?if?employee.role?is?role]

更清晰明確的變量名

舊版本：

@dataclass
class?HourlyEmployee(Employee):???
????"""?Employee?that's?paid?based?on?number?of?worked?hours."""???
????
????hourly_rate:?float?=?50???
????amount:?int?=?10

新版本：

@dataclass
class?HourlyEmployee(Employee):???
????"""?Employee?that's?paid?based?on?number?of?worked?hours."""??
????
????hourly_rate_dollars:?float?=?50??
????hours_worked:?int?=?10

isinstance

當(dāng)你在代碼的任何地方看到 isinstance 這個(gè)函數(shù)時(shí)，都需要特別地加以關(guān)注。它意味著代碼中有可能存在某些有待提升的設(shè)計(jì)。

比如代碼中的 pay_employee 函數(shù)：

def?pay_employee(self,?employee:?Employee)?->?None:???
????if?isinstance(employee,?SalariedEmployee):?????
???????print(???????
???????????f"Paying?employee?{employee.name}?a?monthly?salary?of?${employee.monthly_salary}"???????
???????)??
???elif?isinstance(employee,?HourlyEmployee):?????
???????print(?????
???????????f"Paying?employee?{employee.name}?a?hourly?rate?of?\?????????????????????
???????????????????????${employee.hourly_rate_dollars}?for?{employee.hours_worked}?hours."??????
????????)

這里 isinstance 的使用，實(shí)際上在 pay_employee 函數(shù)中引入了對 Employee 的子類的依賴。這種依賴導(dǎo)致各部分代碼之間的職責(zé)劃分不夠清晰，耦合性變強(qiáng)。

pay_employee 方法需要與 Employee 的子類的具體實(shí)現(xiàn)保持同步。每新增一個(gè)新的員工類型（Employee 的子類），此方法中的 if-else 也就必須再新增一個(gè)分支。即需要同時(shí)改動不同位置的兩部分代碼。

可以將 pay_employee 的實(shí)現(xiàn)從 Company 類轉(zhuǎn)移到具體的 Employee 子類中。即特定類型的員工擁有對應(yīng)的報(bào)酬支付方法，公司在發(fā)薪時(shí)只需要調(diào)用對應(yīng)員工的 pay 方法，無需實(shí)現(xiàn)自己的pay_employee 方法。由 isinstance 引入的依賴關(guān)系從而被移除。

@dataclass
class?HourlyEmployee(Employee):???
????"""?Employee?that's?paid?based?on?number?of?worked?hours."""???
????
????hourly_rate_dollars:?float?=?50??
????hours_worked:?int?=?10??
????
????def?pay(self):???
????????print(????????
????????????f"Paying?employee?{self.name}?a?hourly?rate?of?\???????????????????
????????????????????${self.hourly_rate_dollars}?for?{self.hours_worked}?hours."???????
?????????)
?????????
@dataclass
class?SalariedEmployee(Employee):??
????"""Employee?that's?paid?based?on?a?fixed?monthly?salary."""???
????
????monthly_salary:?float?=?5000??
????
????def?pay(self):???
????????print(????????
????????????f"Paying?employee?{self.name}?a?monthly?salary?of?${self.monthly_salary}"?????
????????)

再把 main 函數(shù)中的 company.pay_employee(company.employees[0]) 改為 company.employees[0].pay()。

由于每一個(gè)特定的 Employee 子類都需要實(shí)現(xiàn) pay 方法，更好的方式是將 Employee 實(shí)現(xiàn)為虛擬基類，pay 成為子類必須實(shí)現(xiàn)的虛擬方法。

from?abc?import?ABC,?abstractmethod


class?Employee(ABC):??

?????@abstractmethod???
?????def?pay()?->?None:?????
?????????"""Method?to?call?when?paying?an?employee"""

Bool flag

Employee 類中的 take_a_holiday 方法有一個(gè)名為 payout 的參數(shù)。它是布爾類型，作為一個(gè)開關(guān)，來決定某個(gè)員工是請一天假，還是以 5 天為單位將假期兌換為報(bào)酬。

這個(gè)開關(guān)實(shí)際上導(dǎo)致了 take_a_holiday 方法包含了兩種不同的職責(zé)，只通過一個(gè)布爾值來決定具體執(zhí)行哪一個(gè)。

函數(shù)原本的目的就是職責(zé)的分離。使得同一個(gè)代碼塊中不會包含過多不同類型的任務(wù)。

因此 take_a_holiday 方法最好分割成兩個(gè)不同的方法，分別應(yīng)對不同的休假方式。

class?Employee(ABC):???

????def?take_a_holiday(self)?->?None:???
????????"""Let?the?employee?take?a?single?holiday."""???
????????
????????if?self.vacation_days?????????????raise?ValueError(?????????
????????????????"You?don't?have?any?holidays?left.?Now?back?to?work,?you!"?????????
????????????)??????
????????self.vacation_days?-=?1????
????????print("Have?fun?on?your?holiday.?Don't?forget?to?check?your?emails!")???
????def?payout_a_holiday(self)?->?None:????
????????"""Let?the?employee?get?paid?for?unused?holidays."""???????
????????
????????if?self.vacation_days????????????raise?ValueError(?????????????
???????????????f"You?don't?have?enough?holidays?left?over?for?a?payout.?\?????????????????????
???????????????????????Remaining?holidays:?{self.vacation_days}"??????????
????????????)????
????????try:?????????
????????????self.vacation_days?-=?FIXED_VACATION_DAYS_PAYOUT???????????
????????????print(??????????
????????????????f"Paying?out?a?holiday.?Holidays?left:?{self.vacation_days}")??????
?????????except?Exception:??????
?????????????pass

Exceptions

payout_a_holiday 方法中有一步 try-except 代碼。但該部分代碼實(shí)際上對 Exception 沒有做任何事。對于 Exception 而言：

如果需要 catch Exception，就 catch 特定類型的某個(gè) Exception，并對其進(jìn)行處理；如果不會對該 Exception 做任何處理，就不要 catch 它。

在此處使用 try-except 會阻止異常向外拋出，導(dǎo)致外部代碼在調(diào)用 payout_a_holiday 時(shí)獲取不到異常信息。此外，使用 Exception 而不是某個(gè)特定類型的異常，會導(dǎo)致所有的異常信息都被屏蔽掉，包括語法錯(cuò)誤、鍵盤中斷等。

因此，去掉上述代碼中的 try-except。

使用自定義 Exception 替代 ValueError

ValueError 是 Python 內(nèi)置的在內(nèi)部出現(xiàn)值錯(cuò)誤時(shí)拋出的異常，并不適合用在自定義的場景中。最好在代碼中定義自己的異常類型。

class?VacationDaysShortageError(Exception):??
????"""Custom?error?that?is?raised?when?not?enough?vacation?days?available."""???
????
????def?__init__(self,?requested_days:?int,?remaining_days:?int,?message:?str)?->?None:?????
????????self.requested_days?=?requested_days???
????????self.remaining_days?=?remaining_days????
????????self.message?=?message????
????????super().__init__(message)

def?payout_a_holiday(self)?->?None:?
????"""Let?the?employee?get?paid?for?unused?holidays."""??
????
????if?self.vacation_days?????????raise?VacationDaysShortageError(????
????????????requested_days=FIXED_VACATION_DAYS_PAYOUT,???
????????????remaining_days=self.vacation_days,?????
????????????message="You?don't?have?enough?holidays?left?over?for?a?payout.")??
????self.vacation_days?-=?FIXED_VACATION_DAYS_PAYOUT??
????print(f"Paying?out?a?holiday.?Holidays?left:?{self.vacation_days}")

最終版本

"""
Very?advanced?Employee?management?system.
"""

from?dataclasses?import?dataclass
from?typing?import?List
from?enum?import?Enum,?auto
from?abc?import?ABC,?abstractmethod

#?The?fixed?number?of?vacation?days?that?can?be?paid?out.
FIXED_VACATION_DAYS_PAYOUT?=?5


class?VacationDaysShortageError(Exception):
????"""Custom?error?that?is?raised?when?not?enough?vacation?days?available."""

????def?__init__(self,?requested_days:?int,?remaining_days:?int,?message:?str)?->?None:
????????self.requested_days?=?requested_days
????????self.remaining_days?=?remaining_days
????????self.message?=?message
????????super().__init__(message)


class?Role(Enum):
????"""Employee?roles."""
????PRESIDENT?=?auto()
????VICEPRESIDENT?=?auto()
????MANAGER?=?auto()
????LEAD?=?auto()
????WORKER?=?auto()
????INTERN?=?auto()


@dataclass
class?Employee(ABC):
????"""Basic?representation?of?an?employee?at?the?company"""

????name:?str
????role:?Role
????vacation_days:?int?=?25

????def?take_a_holiday(self)?->?None:
????????"""Let?the?employee?take?a?single?holiday."""

????????if?self.vacation_days?????????????raise?VacationDaysShortageError(
????????????????requested_days=1,
????????????????remaining_days=self.vacation_days,
????????????????message="You?don't?have?any?holidays?left.?Now?back?to?work,?you!")
????????self.vacation_days?-=?1
????????print("Have?fun?on?your?holiday.?Don't?forget?to?check?your?emails!")

????def?payout_a_holiday(self)?->?None:
????????"""Let?the?employee?get?paid?for?unused?holidays."""

????????if?self.vacation_days?????????????raise?VacationDaysShortageError(
????????????????requested_days=FIXED_VACATION_DAYS_PAYOUT,
????????????????remaining_days=self.vacation_days,
????????????????message="You?don't?have?enough?holidays?left?over?for?a?payout."
????????????)
????????self.vacation_days?-=?FIXED_VACATION_DAYS_PAYOUT
????????print(f"Paying?out?a?holiday.?Holidays?left:?{self.vacation_days}")

????@abstractmethod
????def?pay()?->?None:
????????"""Method?to?call?when?paying?an?employee"""


@dataclass
class?HourlyEmployee(Employee):
????"""?Employee?that's?paid?based?on?number?of?worked?hours."""

????hourly_rate_dollars:?float?=?50
????hours_worked:?int?=?10

????def?pay(self):
????????print(
????????????f"Paying?employee?{self.name}?a?hourly?rate?of?\
????????????????????${self.hourly_rate_dollars}?for?{self.hours_worked}?hours."
????????)


@dataclass
class?SalariedEmployee(Employee):
????"""Employee?that's?paid?based?on?a?fixed?monthly?salary."""

????monthly_salary:?float?=?5000

????def?pay(self):
????????print(
????????????f"Paying?employee?{self.name}?a?monthly?salary?of?${self.monthly_salary}"
????????)


class?Company:
????"""Represents?a?company?with?employees."""

????def?__init__(self)?->?None:
????????self.employees:?List[Employee]?=?[]

????def?add_employee(self,?employee:?Employee)?->?None:
????????self.employees.append(employee)

????def?find_employees(self,?role:?Role)?->?List[Employee]:
????????"""Find?all?employees?with?a?particular?role."""

????????return?[employee?for?employee?in?self.employees?if?employee.role?is?role]


def?main()?->?None:
????company?=?Company()
????company.add_employee(SalariedEmployee(name="Louis",?role=Role.MANAGER))
????company.add_employee(HourlyEmployee(
????????name="Brenda",?role=Role.VICEPRESIDENT))
????company.add_employee(HourlyEmployee(name="Tim",?role=Role.INTERN))
????print(company.find_employees(Role.VICEPRESIDENT))
????print(company.find_employees(Role.MANAGER))
????print(company.find_employees(Role.INTERN))
????company.employees[0].pay()
????company.employees[0].take_a_holiday()


if?__name__?==?'__main__':
????main()

參考資料

7 Python Code Smells: Olfactory Offenses To Avoid At All Costs