Refactoring a Python Employee Management System to Eliminate Code Smells

Code smells are aspects of source code that make developers feel uncomfortable, often indicating poor readability, duplicated logic, or design flaws. The article starts with a simple employee‑management script that suffers from several smells, such as storing the employee role as a plain str , having three almost identical find_managers , find_vice_presidents , and find_interns methods, and using isinstance to decide payment logic.

Original implementation

<code>from dataclasses import dataclass
from typing import List

FIXED_VACATION_DAYS_PAYOUT = 5

@dataclass
class Employee:
    name: str
    role: str
    vacation_days: int = 25
    ...
</code>

The script also mixes two responsibilities in take_a_holiday (single‑day leave vs. payout) and catches a generic Exception without handling it.

Suggested improvements

Use an Enum for roles to avoid misspellings and provide a clear set of possible values.

Consolidate duplicate search methods into a single find_employees(self, role: Role) -> List[Employee] that uses a list comprehension.

Replace isinstance dispatch with polymorphic pay methods on each concrete employee subclass.

Split holiday logic into separate take_a_holiday and payout_a_holiday methods, removing the boolean flag.

Introduce a custom exception ( VacationDaysShortageError ) instead of generic ValueError for clearer error handling.

Make Employee an abstract base class with an abstract pay method, enforcing implementation in subclasses.

Refactored code (final version)

<code>from dataclasses import dataclass
from typing import List
from enum import Enum, auto
from abc import ABC, abstractmethod

FIXED_VACATION_DAYS_PAYOUT = 5

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

class Role(Enum):
    PRESIDENT = auto()
    VICEPRESIDENT = auto()
    MANAGER = auto()
    LEAD = auto()
    WORKER = auto()
    INTERN = auto()

@dataclass
class Employee(ABC):
    name: str
    role: Role
    vacation_days: int = 25

    def take_a_holiday(self) -> None:
        if self.vacation_days < 1:
            raise VacationDaysShortageError(1, self.vacation_days,
                "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:
        if self.vacation_days < FIXED_VACATION_DAYS_PAYOUT:
            raise VacationDaysShortageError(FIXED_VACATION_DAYS_PAYOUT,
                self.vacation_days, "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(self) -> None:
        """Method to call when paying an employee"""

@dataclass
class HourlyEmployee(Employee):
    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):
    monthly_salary: float = 5000

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

class Company:
    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]:
        return [e for e in self.employees if e.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()
</code>

The refactor eliminates duplicated code, improves type safety with Enum , leverages polymorphism to simplify payment logic, uses list comprehensions for concise searching, and replaces vague error handling with explicit custom exceptions, resulting in a cleaner, more maintainable Python codebase.