software architecture
Code Smells Every Developer Should Recognize: The Silent Killers of Software Quality
Learn to identify and eliminate code smells that silently degrade software quality. Practical guide with Python examples showing how to recognize warning signs and implement systematic prevention strategies.
30 min read
✦
Key TakeawayCode smells are the patterns that work today and cost you at 2am six months from now — the 300-line function, the god class, the copy-pasted logic that drifts out of sync. They aren't bugs; they're the design problems that make bugs likely and changes expensive. The skill worth building isn't memorizing a catalogue — it's recognizing the smell in review and knowing what to say about it.
Code Smells Every Developer Should Recognize
You're debugging a critical production issue at 2am. The fix should be one line. But the function you land in is 300 lines long and does authentication, database queries, and email sending in one breath. Variables are named temp1, data, result. The class around it has twenty unrelated methods. The "simple" fix becomes a six-hour investigation, because the code refuses to tell you what it does.
That 300-line function isn't just a style problem. It's the function that takes down the auth flow the night the email provider has an outage — because sending the welcome email and authenticating the user live in the same method, on the same failure path. The coupling is the real smell. The ugliness is just how you spot it.
Code smells are symptoms of design problems. Unlike bugs, they don't fail loudly — they quietly make everything slower and riskier until the team spends more time understanding code than changing it. The term comes from Martin Fowler and Kent Beck, but the practical skill is narrower than the literature: can you name the smell when it shows up in a pull request, and can you say something useful instead of just "this feels off"? Here are the five that do the most damage, in Python, each with the moment you catch it.
The five smells that cost the most
1. Long method — when one function has more than one reason to change
You spot this one by scrolling. If you have to scroll to see the whole function, or you find yourself adding a comment like # now send the email halfway down, that comment is the function telling you it should be two functions. In review I say: "What's this function's one job? I count three — validation, persistence, and notification. Each of those wants its own home so we can test and change them separately."
The danger isn't aesthetic. A method that mixes concerns can't be tested without triggering all of them, and a failure in one (the email server) takes down the others (the registration) because they share a code path.
Smelly Example:
def process_user_registration(username, email, password, profile_data):
"""Process user registration - this method does everything!"""
# Validate username
if not username or len(username) < 3:
print("Username too short")
return False
if username in ['admin', 'root', 'test']:
print("Reserved username")
return False
# Check if username exists
import sqlite3
conn = sqlite3.connect('users.db')
cursor = conn.cursor()
cursor.execute("SELECT id FROM users WHERE username = ?", (username,))
if cursor.fetchone():
print("Username already exists")
conn.close()
return False
# Validate email
if not email or '@' not in email:
print("Invalid email")
conn.close()
return False
# Check email format with regex
import re
email_pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$'
if not re.match(email_pattern, email):
print("Email format invalid")
conn.close()
return False
# Check if email exists
cursor.execute("SELECT id FROM users WHERE email = ?", (email,))
if cursor.fetchone():
print("Email already registered")
conn.close()
return False
# Validate password strength
if len(password) < 8:
print("Password too short")
conn.close()
return False
has_upper = any(c.isupper() for c in password)
has_lower = any(c.islower() for c in password)
has_digit = any(c.isdigit() for c in password)
has_special = any(c in '!@#$%^&*()_+-=[]{}|;:,.<>?' for c in password)
if not (has_upper and has_lower and has_digit and has_special):
print("Password must contain uppercase, lowercase, digit, and special character")
conn.close()
return False
# Hash password
import hashlib
import secrets
salt = secrets.token_hex(16)
password_hash = hashlib.pbkdf2_hmac('sha256', password.encode(), salt.encode(), 100000)
# Validate profile data
required_fields = ['first_name', 'last_name', 'birth_date']
for field in required_fields:
if field not in profile_data or not profile_data[field]:
print(f"Missing required field: {field}")
conn.close()
return False
# Validate birth date
from datetime import datetime, date
try:
birth_date = datetime.strptime(profile_data['birth_date'], '%Y-%m-%d').date()
today = date.today()
age = today.year - birth_date.year - ((today.month, today.day) < (birth_date.month, birth_date.day))
if age < 13:
print("User must be at least 13 years old")
conn.close()
return False
except ValueError:
print("Invalid birth date format")
conn.close()
return False
# Create user record
import json
cursor.execute("""
INSERT INTO users (username, email, password_hash, salt, profile_data, created_at)
VALUES (?, ?, ?, ?, ?, ?)
""", (username, email, password_hash.hex(), salt, json.dumps(profile_data), datetime.now()))
user_id = cursor.lastrowid
conn.commit()
# Create user directory
import os
user_dir = f"user_data/{user_id}"
os.makedirs(user_dir, exist_ok=True)
# Send welcome email
import smtplib
from email.mime.text import MIMEText
try:
msg = MIMEText(f"Welcome {profile_data['first_name']}! Your account has been created.")
msg['Subject'] = 'Welcome to Our Platform'
msg['From'] = 'noreply@company.com'
msg['To'] = email
server = smtplib.SMTP('smtp.gmail.com', 587)
server.starttls()
server.login('noreply@company.com', 'app_password')
server.send_message(msg)
server.quit()
print("Welcome email sent")
except Exception as e:
print(f"Failed to send welcome email: {e}")
# Don't fail registration if email fails
# Log registration
with open('registration_log.txt', 'a') as log_file:
log_file.write(f"{datetime.now()}: User {username} ({email}) registered\n")
# Update statistics
cursor.execute("UPDATE site_stats SET total_users = total_users + 1")
conn.commit()
conn.close()
print(f"User {username} registered successfully")
return TrueClean Refactored Version:
from abc import ABC, abstractmethod
from dataclasses import dataclass
from typing import Dict, List, Optional, Tuple
from datetime import datetime, date
import logging
@dataclass
class UserRegistrationData:
username: str
email: str
password: str
profile_data: Dict[str, str]
@dataclass
class ValidationResult:
is_valid: bool
error_message: Optional[str] = None
class UserValidator:
"""Handles all user data validation logic."""
RESERVED_USERNAMES = {'admin', 'root', 'test', 'administrator'}
MIN_USERNAME_LENGTH = 3
MIN_PASSWORD_LENGTH = 8
MIN_AGE = 13
def __init__(self, user_repository):
self.user_repository = user_repository
self.logger = logging.getLogger(__name__)
def validate_registration_data(self, registration_data: UserRegistrationData) -> ValidationResult:
"""Validate complete registration data."""
# Validate username
username_result = self._validate_username(registration_data.username)
if not username_result.is_valid:
return username_result
# Validate email
email_result = self._validate_email(registration_data.email)
if not email_result.is_valid:
return email_result
# Validate password
password_result = self._validate_password(registration_data.password)
if not password_result.is_valid:
return password_result
# Validate profile data
profile_result = self._validate_profile_data(registration_data.profile_data)
if not profile_result.is_valid:
return profile_result
return ValidationResult(True)
def _validate_username(self, username: str) -> ValidationResult:
if not username or len(username) < self.MIN_USERNAME_LENGTH:
return ValidationResult(False, f"Username must be at least {self.MIN_USERNAME_LENGTH} characters")
if username.lower() in self.RESERVED_USERNAMES:
return ValidationResult(False, "Username is reserved")
if self.user_repository.username_exists(username):
return ValidationResult(False, "Username already exists")
return ValidationResult(True)
def _validate_email(self, email: str) -> ValidationResult:
if not email or '@' not in email:
return ValidationResult(False, "Invalid email address")
# Use proper email validation library in production
import re
email_pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$'
if not re.match(email_pattern, email):
return ValidationResult(False, "Email format is invalid")
if self.user_repository.email_exists(email):
return ValidationResult(False, "Email already registered")
return ValidationResult(True)
def _validate_password(self, password: str) -> ValidationResult:
if len(password) < self.MIN_PASSWORD_LENGTH:
return ValidationResult(False, f"Password must be at least {self.MIN_PASSWORD_LENGTH} characters")
requirements = [
(any(c.isupper() for c in password), "uppercase letter"),
(any(c.islower() for c in password), "lowercase letter"),
(any(c.isdigit() for c in password), "digit"),
(any(c in '!@#$%^&*()_+-=[]{}|;:,.<>?' for c in password), "special character")
]
missing_requirements = [req for has_req, req in requirements if not has_req]
if missing_requirements:
return ValidationResult(
False,
f"Password must contain: {', '.join(missing_requirements)}"
)
return ValidationResult(True)
def _validate_profile_data(self, profile_data: Dict[str, str]) -> ValidationResult:
required_fields = ['first_name', 'last_name', 'birth_date']
for field in required_fields:
if field not in profile_data or not profile_data[field]:
return ValidationResult(False, f"Missing required field: {field}")
return self._validate_birth_date(profile_data['birth_date'])
def _validate_birth_date(self, birth_date_str: str) -> ValidationResult:
try:
birth_date = datetime.strptime(birth_date_str, '%Y-%m-%d').date()
today = date.today()
age = today.year - birth_date.year - ((today.month, today.day) < (birth_date.month, birth_date.day))
if age < self.MIN_AGE:
return ValidationResult(False, f"User must be at least {self.MIN_AGE} years old")
return ValidationResult(True)
except ValueError:
return ValidationResult(False, "Invalid birth date format (use YYYY-MM-DD)")
class PasswordService:
"""Handles password hashing and security."""
def hash_password(self, password: str) -> Tuple[str, str]:
"""Hash password and return (hash, salt) tuple."""
import hashlib
import secrets
salt = secrets.token_hex(16)
password_hash = hashlib.pbkdf2_hmac('sha256', password.encode(), salt.encode(), 100000)
return password_hash.hex(), salt
class UserRegistrationService:
"""Main service for handling user registration."""
def __init__(self,
user_repository,
email_service,
file_service,
analytics_service):
self.user_repository = user_repository
self.email_service = email_service
self.file_service = file_service
self.analytics_service = analytics_service
self.validator = UserValidator(user_repository)
self.password_service = PasswordService()
self.logger = logging.getLogger(__name__)
def register_user(self, registration_data: UserRegistrationData) -> RegistrationResult:
"""Register a new user with complete validation and setup."""
# Validate registration data
validation_result = self.validator.validate_registration_data(registration_data)
if not validation_result.is_valid:
return RegistrationResult(False, validation_result.error_message)
try:
# Hash password
password_hash, salt = self.password_service.hash_password(registration_data.password)
# Create user
user = self.user_repository.create_user(
username=registration_data.username,
email=registration_data.email,
password_hash=password_hash,
salt=salt,
profile_data=registration_data.profile_data
)
# Setup user environment
self._setup_user_environment(user)
# Send welcome email (async, don't block registration)
self.email_service.send_welcome_email_async(user)
# Update analytics
self.analytics_service.record_user_registration(user)
self.logger.info(f"User {user.username} registered successfully")
return RegistrationResult(True, "Registration successful", user)
except Exception as e:
self.logger.exception(f"Failed to register user {registration_data.username}")
return RegistrationResult(False, "Registration failed. Please try again.")
def _setup_user_environment(self, user):
"""Setup initial user environment (directories, etc.)."""
try:
self.file_service.create_user_directory(user.id)
except Exception as e:
self.logger.warning(f"Failed to create user directory for {user.id}: {e}")
# Don't fail registration for directory creation issues
@dataclass
class RegistrationResult:
success: bool
message: str
user: Optional['User'] = NoneKey Improvements:
- Single Responsibility: Each class has a clear, focused purpose
- Testability: Each component can be tested independently
- Maintainability: Changes to validation logic don't affect email sending
- Readability: The flow is clear and business logic is explicit
- Error Handling: Proper exception handling with clear recovery paths
2. Large class — when the method names stop rhyming
You catch the god object by reading the method list, not the methods. When a UserManager has create_user, send_email, upload_profile_picture, generate_report, and backup_user_data, the names have stopped rhyming — they're from five different domains wearing one class. In review: "This class has five jobs. Persistence, auth, email, files, reporting — each of those is a class, and the day we change how email works we shouldn't have to open the user code to do it."
The cost shows up later as tight coupling: everything depends on the god object, so nothing can be tested or changed in isolation.
Smelly Example:
class UserManager:
"""This class handles EVERYTHING related to users... and more!"""
def __init__(self):
self.db_connection = sqlite3.connect('app.db')
self.email_config = {
'smtp_server': 'smtp.gmail.com',
'port': 587,
'username': 'admin@company.com',
'password': 'secret123'
}
# User CRUD operations
def create_user(self, username, email, password):
# 50 lines of user creation logic
pass
def update_user(self, user_id, data):
# 40 lines of user update logic
pass
def delete_user(self, user_id):
# 30 lines of user deletion logic
pass
def get_user(self, user_id):
# 20 lines of user retrieval logic
pass
# Authentication
def login_user(self, username, password):
# 60 lines of authentication logic
pass
def logout_user(self, session_id):
# 30 lines of session cleanup
pass
def reset_password(self, email):
# 70 lines of password reset logic
pass
# Email operations
def send_welcome_email(self, user):
# 40 lines of email sending logic
pass
def send_password_reset_email(self, user, reset_token):
# 50 lines of email logic
pass
def send_notification_email(self, user, message):
# 45 lines of notification logic
pass
# File operations
def upload_profile_picture(self, user_id, file_data):
# 60 lines of file upload logic
pass
def delete_user_files(self, user_id):
# 40 lines of file deletion logic
pass
# Analytics and reporting
def generate_user_report(self, date_range):
# 80 lines of reporting logic
pass
def track_user_activity(self, user_id, activity):
# 30 lines of activity tracking
pass
# Settings management
def update_user_preferences(self, user_id, preferences):
# 50 lines of preference logic
pass
def get_user_settings(self, user_id):
# 40 lines of settings retrieval
pass
# Validation methods
def validate_email(self, email):
# 20 lines of email validation
pass
def validate_username(self, username):
# 25 lines of username validation
pass
# Database helper methods
def backup_user_data(self):
# 100 lines of backup logic
pass
def cleanup_old_sessions(self):
# 60 lines of cleanup logic
passClean Refactored Version:
# Separate concerns into focused classes
class User:
"""Domain model representing a user."""
def __init__(self, id, username, email, created_at, profile_data=None):
self.id = id
self.username = username
self.email = email
self.created_at = created_at
self.profile_data = profile_data or {}
class UserRepository:
"""Handles user data persistence."""
def __init__(self, database_service):
self.db = database_service
def create_user(self, username: str, email: str, password_hash: str) -> User:
"""Create a new user in the database."""
query = """
INSERT INTO users (username, email, password_hash, created_at)
VALUES (?, ?, ?, ?)
"""
user_id = self.db.execute_and_return_id(
query, (username, email, password_hash, datetime.now())
)
return User(user_id, username, email, datetime.now())
def get_user_by_id(self, user_id: int) -> Optional[User]:
"""Retrieve user by ID."""
query = "SELECT id, username, email, created_at FROM users WHERE id = ?"
result = self.db.fetch_one(query, (user_id,))
if result:
return User(*result)
return None
def get_user_by_username(self, username: str) -> Optional[User]:
"""Retrieve user by username."""
query = "SELECT id, username, email, created_at FROM users WHERE username = ?"
result = self.db.fetch_one(query, (username,))
if result:
return User(*result)
return None
def update_user(self, user_id: int, updates: Dict[str, Any]) -> bool:
"""Update user data."""
if not updates:
return True
set_clause = ", ".join([f"{key} = ?" for key in updates.keys()])
query = f"UPDATE users SET {set_clause} WHERE id = ?"
return self.db.execute(query, (*updates.values(), user_id))
def delete_user(self, user_id: int) -> bool:
"""Delete user from database."""
query = "DELETE FROM users WHERE id = ?"
return self.db.execute(query, (user_id,))
class AuthenticationService:
"""Handles user authentication and session management."""
def __init__(self, user_repository, password_service, session_service):
self.user_repository = user_repository
self.password_service = password_service
self.session_service = session_service
def authenticate_user(self, username: str, password: str) -> Optional[User]:
"""Authenticate user credentials."""
user = self.user_repository.get_user_by_username(username)
if not user:
return None
if self.password_service.verify_password(password, user.password_hash):
return user
return None
def create_session(self, user: User) -> str:
"""Create a new user session."""
return self.session_service.create_session(user.id)
def logout_user(self, session_id: str) -> bool:
"""Logout user and cleanup session."""
return self.session_service.destroy_session(session_id)
class EmailService:
"""Handles all email communications."""
def __init__(self, smtp_config):
self.smtp_config = smtp_config
self.logger = logging.getLogger(__name__)
def send_welcome_email(self, user: User) -> bool:
"""Send welcome email to new user."""
subject = "Welcome to Our Platform!"
body = f"Hello {user.username}, welcome to our platform!"
return self._send_email(user.email, subject, body)
def send_password_reset_email(self, user: User, reset_token: str) -> bool:
"""Send password reset email."""
subject = "Password Reset Request"
reset_link = f"https://oursite.com/reset-password?token={reset_token}"
body = f"Click here to reset your password: {reset_link}"
return self._send_email(user.email, subject, body)
def _send_email(self, to_email: str, subject: str, body: str) -> bool:
"""Internal method to send email."""
try:
# Email sending implementation
self.logger.info(f"Email sent to {to_email}: {subject}")
return True
except Exception as e:
self.logger.error(f"Failed to send email to {to_email}: {e}")
return False
class UserFileService:
"""Handles user file operations."""
def __init__(self, storage_service):
self.storage = storage_service
def upload_profile_picture(self, user_id: int, file_data: bytes) -> str:
"""Upload user profile picture."""
filename = f"profile_{user_id}_{datetime.now().timestamp()}.jpg"
file_path = f"users/{user_id}/profile/{filename}"
self.storage.save_file(file_path, file_data)
return file_path
def delete_user_files(self, user_id: int) -> bool:
"""Delete all files for a user."""
user_directory = f"users/{user_id}/"
return self.storage.delete_directory(user_directory)
class UserService:
"""Main service orchestrating user operations."""
def __init__(self,
user_repository,
auth_service,
email_service,
file_service):
self.user_repository = user_repository
self.auth_service = auth_service
self.email_service = email_service
self.file_service = file_service
def register_user(self, username: str, email: str, password: str) -> User:
"""Register a new user with complete setup."""
# Create user
user = self.user_repository.create_user(username, email, password)
# Send welcome email
self.email_service.send_welcome_email(user)
return user
def delete_user_account(self, user_id: int) -> bool:
"""Delete user account and cleanup all associated data."""
# Delete user files
self.file_service.delete_user_files(user_id)
# Delete user from database
return self.user_repository.delete_user(user_id)3. Duplicate code — when a bug fix has to be applied twice
You recognize this one when you fix a bug and then get a nagging feeling you've seen that code somewhere else. The calculate_employee_bonus and calculate_contractor_bonus functions below are 80% the same logic with different numbers — which means the next change to the bonus rule has to land in both, and the day someone forgets, the two quietly disagree. In review: "These two share a shape. Let's pull the structure out so the rules live in one place and the differences become data, not duplicated branches."
A caution: not all repetition is duplication. Two things that look alike today but change for different reasons should stay apart. The smell is shared logic that will change together, not coincidental resemblance.
Smelly Example:
def calculate_employee_bonus(employee):
base_salary = employee['salary']
years_experience = employee['years_experience']
performance_rating = employee['performance_rating']
# Junior employee bonus calculation
if years_experience < 3:
if performance_rating >= 4.5:
bonus = base_salary * 0.15
elif performance_rating >= 4.0:
bonus = base_salary * 0.10
elif performance_rating >= 3.5:
bonus = base_salary * 0.05
else:
bonus = 0
if employee['department'] == 'Engineering':
bonus += 1000
elif employee['department'] == 'Sales':
bonus += 800
return min(bonus, 10000) # Cap at 10k
# Senior employee bonus calculation
elif years_experience < 8:
if performance_rating >= 4.5:
bonus = base_salary * 0.20
elif performance_rating >= 4.0:
bonus = base_salary * 0.15
elif performance_rating >= 3.5:
bonus = base_salary * 0.10
else:
bonus = 0
if employee['department'] == 'Engineering':
bonus += 2000
elif employee['department'] == 'Sales':
bonus += 1500
return min(bonus, 25000) # Cap at 25k
# Executive employee bonus calculation
else:
if performance_rating >= 4.5:
bonus = base_salary * 0.30
elif performance_rating >= 4.0:
bonus = base_salary * 0.25
elif performance_rating >= 3.5:
bonus = base_salary * 0.15
else:
bonus = 0
if employee['department'] == 'Engineering':
bonus += 5000
elif employee['department'] == 'Sales':
bonus += 4000
return min(bonus, 50000) # Cap at 50k
def calculate_contractor_bonus(contractor):
hourly_rate = contractor['hourly_rate']
hours_worked = contractor['hours_worked']
performance_rating = contractor['performance_rating']
# Similar logic with different rates
if hours_worked < 1000:
if performance_rating >= 4.5:
bonus = hourly_rate * hours_worked * 0.10
elif performance_rating >= 4.0:
bonus = hourly_rate * hours_worked * 0.08
elif performance_rating >= 3.5:
bonus = hourly_rate * hours_worked * 0.05
else:
bonus = 0
if contractor['department'] == 'Engineering':
bonus += 500
elif contractor['department'] == 'Sales':
bonus += 400
return min(bonus, 3000)
else:
if performance_rating >= 4.5:
bonus = hourly_rate * hours_worked * 0.15
elif performance_rating >= 4.0:
bonus = hourly_rate * hours_worked * 0.12
elif performance_rating >= 3.5:
bonus = hourly_rate * hours_worked * 0.08
else:
bonus = 0
if contractor['department'] == 'Engineering':
bonus += 1000
elif contractor['department'] == 'Sales':
bonus += 800
return min(bonus, 8000)Clean Refactored Version:
from abc import ABC, abstractmethod
from dataclasses import dataclass
from enum import Enum
from typing import Dict, Protocol
class Department(Enum):
ENGINEERING = 'Engineering'
SALES = 'Sales'
MARKETING = 'Marketing'
HR = 'HR'
class ExperienceLevel(Enum):
JUNIOR = 'junior'
SENIOR = 'senior'
EXECUTIVE = 'executive'
@dataclass
class BonusConfiguration:
"""Configuration for bonus calculation rules."""
performance_multipliers: Dict[float, float] # performance_threshold -> multiplier
department_bonuses: Dict[Department, float]
bonus_cap: float
def get_performance_multiplier(self, rating: float) -> float:
"""Get bonus multiplier based on performance rating."""
for threshold in sorted(self.performance_multipliers.keys(), reverse=True):
if rating >= threshold:
return self.performance_multipliers[threshold]
return 0.0
def get_department_bonus(self, department: Department) -> float:
"""Get fixed department bonus."""
return self.department_bonuses.get(department, 0.0)
class BonusCalculationStrategy(ABC):
"""Abstract strategy for bonus calculation."""
@abstractmethod
def calculate_base_amount(self, worker_data: Dict) -> float:
"""Calculate the base amount for bonus calculation."""
pass
@abstractmethod
def get_bonus_configuration(self, worker_data: Dict) -> BonusConfiguration:
"""Get bonus configuration based on worker data."""
pass
class EmployeeBonusStrategy(BonusCalculationStrategy):
"""Bonus calculation strategy for employees."""
# Configuration data - could be loaded from database/config
BONUS_CONFIGS = {
ExperienceLevel.JUNIOR: BonusConfiguration(
performance_multipliers={4.5: 0.15, 4.0: 0.10, 3.5: 0.05},
department_bonuses={Department.ENGINEERING: 1000, Department.SALES: 800},
bonus_cap=10000
),
ExperienceLevel.SENIOR: BonusConfiguration(
performance_multipliers={4.5: 0.20, 4.0: 0.15, 3.5: 0.10},
department_bonuses={Department.ENGINEERING: 2000, Department.SALES: 1500},
bonus_cap=25000
),
ExperienceLevel.EXECUTIVE: BonusConfiguration(
performance_multipliers={4.5: 0.30, 4.0: 0.25, 3.5: 0.15},
department_bonuses={Department.ENGINEERING: 5000, Department.SALES: 4000},
bonus_cap=50000
)
}
def calculate_base_amount(self, employee_data: Dict) -> float:
"""For employees, base amount is salary."""
return employee_data['salary']
def get_bonus_configuration(self, employee_data: Dict) -> BonusConfiguration:
"""Get configuration based on experience level."""
years_experience = employee_data['years_experience']
if years_experience < 3:
level = ExperienceLevel.JUNIOR
elif years_experience < 8:
level = ExperienceLevel.SENIOR
else:
level = ExperienceLevel.EXECUTIVE
return self.BONUS_CONFIGS[level]
class ContractorBonusStrategy(BonusCalculationStrategy):
"""Bonus calculation strategy for contractors."""
BONUS_CONFIGS = {
'low_hours': BonusConfiguration(
performance_multipliers={4.5: 0.10, 4.0: 0.08, 3.5: 0.05},
department_bonuses={Department.ENGINEERING: 500, Department.SALES: 400},
bonus_cap=3000
),
'high_hours': BonusConfiguration(
performance_multipliers={4.5: 0.15, 4.0: 0.12, 3.5: 0.08},
department_bonuses={Department.ENGINEERING: 1000, Department.SALES: 800},
bonus_cap=8000
)
}
def calculate_base_amount(self, contractor_data: Dict) -> float:
"""For contractors, base amount is total earnings."""
return contractor_data['hourly_rate'] * contractor_data['hours_worked']
def get_bonus_configuration(self, contractor_data: Dict) -> BonusConfiguration:
"""Get configuration based on hours worked."""
hours_worked = contractor_data['hours_worked']
if hours_worked < 1000:
return self.BONUS_CONFIGS['low_hours']
else:
return self.BONUS_CONFIGS['high_hours']
class BonusCalculator:
"""Main service for calculating bonuses using strategy pattern."""
def __init__(self, strategy: BonusCalculationStrategy):
self.strategy = strategy
def calculate_bonus(self, worker_data: Dict) -> float:
"""Calculate bonus using the configured strategy."""
# Get base amount and configuration
base_amount = self.strategy.calculate_base_amount(worker_data)
config = self.strategy.get_bonus_configuration(worker_data)
# Calculate performance-based bonus
performance_rating = worker_data['performance_rating']
performance_multiplier = config.get_performance_multiplier(performance_rating)
performance_bonus = base_amount * performance_multiplier
# Add department bonus
department = Department(worker_data['department'])
department_bonus = config.get_department_bonus(department)
# Calculate total bonus
total_bonus = performance_bonus + department_bonus
# Apply cap
return min(total_bonus, config.bonus_cap)
# Usage examples
def calculate_employee_bonus(employee_data: Dict) -> float:
calculator = BonusCalculator(EmployeeBonusStrategy())
return calculator.calculate_bonus(employee_data)
def calculate_contractor_bonus(contractor_data: Dict) -> float:
calculator = BonusCalculator(ContractorBonusStrategy())
return calculator.calculate_bonus(contractor_data)
# Easy to extend for new worker types
class InternBonusStrategy(BonusCalculationStrategy):
"""New strategy for intern bonuses."""
def calculate_base_amount(self, intern_data: Dict) -> float:
return intern_data['monthly_stipend'] * intern_data['months_worked']
def get_bonus_configuration(self, intern_data: Dict) -> BonusConfiguration:
return BonusConfiguration(
performance_multipliers={4.5: 0.05, 4.0: 0.03, 3.5: 0.01},
department_bonuses={Department.ENGINEERING: 200, Department.SALES: 150},
bonus_cap=1000
)4. Feature envy — when a method keeps reaching into another object
You spot feature envy when a method mentions another object more than its own. The process_order below barely touches the order — it's elbow-deep in customer.membership_type, customer.join_date, customer.total_lifetime_spending. That logic is envious of the Customer class; it wants to live there. In review: "This discount math is all about the customer. Move it onto Customer and let the order just ask customer.calculate_loyalty_discount()."
The cost is coupling: when the customer's internals change, the order processor breaks, even though orders had nothing to do with the change.
Smelly Example:
class OrderProcessor:
def __init__(self, order_repository, customer_service):
self.order_repository = order_repository
self.customer_service = customer_service
def process_order(self, order_id):
order = self.order_repository.get_order(order_id)
customer = self.customer_service.get_customer(order.customer_id)
# This method is obsessed with customer details!
# It knows too much about customer internal structure
# Calculate customer loyalty discount
if customer.membership_type == 'premium':
base_discount = 0.15
elif customer.membership_type == 'gold':
base_discount = 0.10
elif customer.membership_type == 'silver':
base_discount = 0.05
else:
base_discount = 0.0
# Apply tenure bonus
years_as_customer = (datetime.now() - customer.join_date).days / 365
if years_as_customer > 5:
tenure_bonus = 0.05
elif years_as_customer > 2:
tenure_bonus = 0.02
else:
tenure_bonus = 0.0
total_discount = base_discount + tenure_bonus
# Apply spending tier bonus
if customer.total_lifetime_spending > 50000:
spending_bonus = 0.03
elif customer.total_lifetime_spending > 20000:
spending_bonus = 0.02
elif customer.total_lifetime_spending > 5000:
spending_bonus = 0.01
else:
spending_bonus = 0.0
total_discount += spending_bonus
total_discount = min(total_discount, 0.25) # Cap at 25%
# Calculate order total
order_total = sum(item.price * item.quantity for item in order.items)
discount_amount = order_total * total_discount
final_total = order_total - discount_amount
# More customer-specific logic
if customer.preferred_payment_method == 'credit_card':
# Apply credit card processing fee
final_total += final_total * 0.03
elif customer.preferred_payment_method == 'bank_transfer':
# Bank transfer discount
final_total -= 10.0
return final_totalClean Refactored Version:
from dataclasses import dataclass
from datetime import datetime, date
from typing import List, Optional
@dataclass
class Customer:
id: str
membership_type: str
join_date: date
total_lifetime_spending: float
preferred_payment_method: str
def calculate_loyalty_discount(self) -> float:
"""Calculate customer loyalty discount based on membership and history."""
# Base membership discount
membership_discounts = {
'premium': 0.15,
'gold': 0.10,
'silver': 0.05
}
base_discount = membership_discounts.get(self.membership_type, 0.0)
# Tenure bonus
years_as_customer = (date.today() - self.join_date).days / 365
if years_as_customer > 5:
tenure_bonus = 0.05
elif years_as_customer > 2:
tenure_bonus = 0.02
else:
tenure_bonus = 0.0
# Spending tier bonus
if self.total_lifetime_spending > 50000:
spending_bonus = 0.03
elif self.total_lifetime_spending > 20000:
spending_bonus = 0.02
elif self.total_lifetime_spending > 5000:
spending_bonus = 0.01
else:
spending_bonus = 0.0
total_discount = base_discount + tenure_bonus + spending_bonus
return min(total_discount, 0.25) # Cap at 25%
def calculate_payment_adjustment(self, amount: float) -> float:
"""Calculate payment method adjustments."""
if self.preferred_payment_method == 'credit_card':
return amount * 0.03 # Processing fee
elif self.preferred_payment_method == 'bank_transfer':
return -10.0 # Discount for bank transfer
else:
return 0.0
@dataclass
class OrderItem:
product_id: str
price: float
quantity: int
@property
def subtotal(self) -> float:
return self.price * self.quantity
@dataclass
class Order:
id: str
customer_id: str
items: List[OrderItem]
@property
def subtotal(self) -> float:
return sum(item.subtotal for item in self.items)
class OrderProcessor:
"""Focused on order processing logic, not customer details."""
def __init__(self, order_repository, customer_service):
self.order_repository = order_repository
self.customer_service = customer_service
def process_order(self, order_id: str) -> float:
"""Process order with clean separation of concerns."""
# Get order and customer
order = self.order_repository.get_order(order_id)
customer = self.customer_service.get_customer(order.customer_id)
# Calculate base order total
order_total = order.subtotal
# Apply customer-specific discount (customer knows how to calculate this)
discount_rate = customer.calculate_loyalty_discount()
discount_amount = order_total * discount_rate
# Calculate total after discount
total_after_discount = order_total - discount_amount
# Apply payment method adjustments (customer knows their preferences)
payment_adjustment = customer.calculate_payment_adjustment(total_after_discount)
final_total = total_after_discount + payment_adjustment
return final_total5. Data clumps — when the same parameters always travel together
You catch this when you find yourself copying the same five or six parameters from one function signature to the next: street_address, city, state, zip_code, country showing up everywhere, together, like they're holding hands. That recurring group is an object that hasn't been born yet. In review: "These five always travel together — that's an Address. Let's make the type so we stop passing them one by one and can give Address its own behavior, like format_full_address()."
The cost is missing abstraction: behavior that belongs to the clump gets scattered across every function that handles it.
Smelly Example:
def create_user_account(username, email, password,
first_name, last_name, birth_date,
street_address, city, state, zip_code, country,
phone_number, emergency_contact_name,
emergency_contact_phone, emergency_contact_relationship):
# All these parameters keep appearing together!
pass
def update_user_profile(user_id, first_name, last_name, birth_date,
street_address, city, state, zip_code, country,
phone_number, emergency_contact_name,
emergency_contact_phone, emergency_contact_relationship):
# Same group of parameters again!
pass
def send_user_notification(username, email, phone_number,
first_name, last_name,
street_address, city, state, zip_code):
# Subset of the same data clump
pass
def generate_user_report(user_ids):
users_data = []
for user_id in user_ids:
# Fetching the same group of fields repeatedly
user = db.fetch_user(user_id)
user_info = {
'first_name': user['first_name'],
'last_name': user['last_name'],
'birth_date': user['birth_date'],
'street_address': user['street_address'],
'city': user['city'],
'state': user['state'],
'zip_code': user['zip_code'],
'country': user['country'],
'phone_number': user['phone_number'],
'emergency_contact_name': user['emergency_contact_name'],
'emergency_contact_phone': user['emergency_contact_phone'],
'emergency_contact_relationship': user['emergency_contact_relationship']
}
users_data.append(user_info)
return users_dataClean Refactored Version:
from dataclasses import dataclass
from datetime import date
from typing import Optional
@dataclass
class PersonalInfo:
"""Represents personal information data clump."""
first_name: str
last_name: str
birth_date: date
@property
def full_name(self) -> str:
return f"{self.first_name} {self.last_name}"
@property
def age(self) -> int:
today = date.today()
return today.year - self.birth_date.year - (
(today.month, today.day) < (self.birth_date.month, self.birth_date.day)
)
@dataclass
class Address:
"""Represents address information data clump."""
street_address: str
city: str
state: str
zip_code: str
country: str
def format_full_address(self) -> str:
return f"{self.street_address}, {self.city}, {self.state} {self.zip_code}, {self.country}"
def is_domestic(self, home_country: str = "USA") -> bool:
return self.country.upper() == home_country.upper()
@dataclass
class EmergencyContact:
"""Represents emergency contact data clump."""
name: str
phone: str
relationship: str
def format_contact_info(self) -> str:
return f"{self.name} ({self.relationship}): {self.phone}"
@dataclass
class ContactInfo:
"""Represents contact information."""
email: str
phone_number: str
address: Address
def get_primary_contact_method(self) -> str:
# Business logic for determining preferred contact method
return "email" # Could be more sophisticated
@dataclass
class User:
"""Clean user representation with organized data."""
id: Optional[str]
username: str
personal_info: PersonalInfo
contact_info: ContactInfo
emergency_contact: EmergencyContact
def get_display_name(self) -> str:
return self.personal_info.full_name
def is_adult(self) -> bool:
return self.personal_info.age >= 18
def get_mailing_address(self) -> str:
return self.contact_info.address.format_full_address()
class UserService:
"""Clean service methods with organized parameters."""
def create_user_account(self, username: str, password: str, user_data: User) -> User:
"""Create user account with organized data structures."""
# Validation and creation logic
user_data.id = self._generate_user_id()
return self._save_user(user_data)
def update_user_profile(self, user_id: str, updates: User) -> bool:
"""Update user profile with clean data structures."""
existing_user = self._get_user(user_id)
if not existing_user:
return False
# Update fields that are provided
if updates.personal_info:
existing_user.personal_info = updates.personal_info
if updates.contact_info:
existing_user.contact_info = updates.contact_info
if updates.emergency_contact:
existing_user.emergency_contact = updates.emergency_contact
return self._save_user(existing_user)
def send_user_notification(self, user: User, message: str) -> bool:
"""Send notification using organized contact information."""
contact_method = user.contact_info.get_primary_contact_method()
if contact_method == "email":
return self._send_email(user.contact_info.email, message)
elif contact_method == "sms":
return self._send_sms(user.contact_info.phone_number, message)
return False
def generate_user_report(self, user_ids: List[str]) -> List[Dict]:
"""Generate report with clean data access."""
users = [self._get_user(user_id) for user_id in user_ids]
return [
{
'name': user.get_display_name(),
'age': user.personal_info.age,
'address': user.get_mailing_address(),
'contact': user.contact_info.email,
'emergency_contact': user.emergency_contact.format_contact_info()
}
for user in users if user
]Advanced Code Smell Detection and Prevention
Beyond recognizing individual smells, mature development teams implement systematic approaches to prevent and detect code quality issues before they become problematic.
Automated Code Smell Detection
Static Analysis Tools:
# Example using pylint configuration for smell detection
# .pylintrc configuration
"""
[MESSAGES CONTROL]
disable=C0103,C0111,C0301,W0613,R0903,W0622,R0913,R0902,R0914,R0915,C0302
[DESIGN]
max-args=5 # Detect parameter lists that are too long
max-locals=15 # Detect functions with too many local variables
max-returns=6 # Detect functions with too many return statements
max-branches=12 # Detect functions with too many branches
max-statements=50 # Detect functions that are too long
max-parents=7 # Detect classes with too many parent classes
max-attributes=7 # Detect classes with too many instance attributes
min-public-methods=2 # Detect classes with too few public methods
max-public-methods=20 # Detect classes with too many public methods
max-bool-expr=5 # Detect boolean expressions that are too complex
"""Custom Code Smell Detectors:
import ast
from typing import List, Dict, Any
class CodeSmellDetector(ast.NodeVisitor):
"""Custom AST visitor to detect specific code smells."""
def __init__(self):
self.smells = []
self.current_class = None
self.current_function = None
def visit_FunctionDef(self, node):
"""Detect function-related smells."""
self.current_function = node.name
# Detect long parameter lists
if len(node.args.args) > 5:
self.smells.append({
'type': 'long_parameter_list',
'location': f"Function {node.name} at line {node.lineno}",
'severity': 'medium',
'description': f"Function has {len(node.args.args)} parameters (max recommended: 5)"
})
# Detect long functions (simplified - count statements)
statement_count = len([n for n in ast.walk(node) if isinstance(n, ast.stmt)])
if statement_count > 50:
self.smells.append({
'type': 'long_method',
'location': f"Function {node.name} at line {node.lineno}",
'severity': 'high',
'description': f"Function has {statement_count} statements (max recommended: 50)"
})
self.generic_visit(node)
self.current_function = None
def visit_ClassDef(self, node):
"""Detect class-related smells."""
self.current_class = node.name
# Count methods in class
methods = [n for n in node.body if isinstance(n, ast.FunctionDef)]
if len(methods) > 15:
self.smells.append({
'type': 'large_class',
'location': f"Class {node.name} at line {node.lineno}",
'severity': 'high',
'description': f"Class has {len(methods)} methods (max recommended: 15)"
})
# Detect potential god classes by looking for diverse method names
method_names = [method.name for method in methods]
diverse_prefixes = set()
for name in method_names:
if '_' in name:
diverse_prefixes.add(name.split('_')[0])
if len(diverse_prefixes) > 5:
self.smells.append({
'type': 'god_class',
'location': f"Class {node.name} at line {node.lineno}",
'severity': 'high',
'description': f"Class appears to handle multiple responsibilities (method prefixes: {diverse_prefixes})"
})
self.generic_visit(node)
self.current_class = None
def visit_If(self, node):
"""Detect deeply nested conditions."""
depth = self._calculate_nesting_depth(node)
if depth > 3:
self.smells.append({
'type': 'deep_nesting',
'location': f"Conditional at line {node.lineno}",
'severity': 'medium',
'description': f"Deeply nested condition (depth: {depth}, max recommended: 3)"
})
self.generic_visit(node)
def _calculate_nesting_depth(self, node, current_depth=0):
"""Calculate maximum nesting depth of conditional statements."""
max_depth = current_depth
for child in ast.iter_child_nodes(node):
if isinstance(child, (ast.If, ast.For, ast.While, ast.With)):
child_depth = self._calculate_nesting_depth(child, current_depth + 1)
max_depth = max(max_depth, child_depth)
return max_depth
def analyze_code_smells(source_code: str) -> List[Dict[str, Any]]:
"""Analyze source code for common smells."""
tree = ast.parse(source_code)
detector = CodeSmellDetector()
detector.visit(tree)
return detector.smells
# Usage example
if __name__ == "__main__":
sample_code = """
def process_user_data(user_id, username, email, first_name, last_name,
address, phone, emergency_contact, preferences, settings):
# This function has too many parameters
if user_id:
if username:
if email:
if first_name:
if last_name:
# Too deeply nested
return True
return False
class UserManager:
def create_user(self): pass
def update_user(self): pass
def delete_user(self): pass
def send_email(self): pass
def upload_file(self): pass
def process_payment(self): pass
def generate_report(self): pass
def backup_data(self): pass
def validate_input(self): pass
def log_activity(self): pass
def calculate_metrics(self): pass
def format_output(self): pass
def handle_errors(self): pass
def authenticate_user(self): pass
def authorize_action(self): pass
def encrypt_data(self): pass
# Too many methods - likely a god class
"""
smells = analyze_code_smells(sample_code)
for smell in smells:
print(f"{smell['severity'].upper()}: {smell['type']}")
print(f" Location: {smell['location']}")
print(f" Description: {smell['description']}\n")Code Review Checklist for Smell Detection
Function-Level Smells:
- Function has a single, clear responsibility
- Function name clearly describes what it does
- Parameter list is reasonable (< 6 parameters)
- Function body fits on one screen (< 50 lines)
- Nesting depth is reasonable (< 4 levels)
- No duplicate code blocks
Class-Level Smells:
- Class has a cohesive set of responsibilities
- Class name clearly describes its purpose
- Number of methods is reasonable (< 20)
- Methods are related to the class's main purpose
- No excessive coupling with other classes
- No god object patterns
System-Level Smells:
- Clear separation of concerns between modules
- Dependencies flow in the right direction
- No circular dependencies
- Appropriate abstraction levels
- Consistent patterns across the codebase
A caution on over-refactoring
Every smell here has an opposite failure mode, and chasing the smell blindly lands you there. Split a method too far and you get a flock of one-line functions you have to chase across five files to follow one flow. Extract a class too eagerly and you get indirection with no payoff. Deduplicate two things that only looked alike and you've coupled them so the next change to one breaks the other.
The test is always the same, and it's not the linter: would the next person understand and change this faster? A smell is a prompt to look, not a command to refactor. Sometimes you look, decide the 60-line function is genuinely one coherent thing, and leave it. That's a valid outcome — the goal was never a clean report, it was cheap change.
What to do Monday morning
- In your next code review, when something feels off, name the smell and say the consequence out loud: "this is feature envy — when the customer changes, this breaks." Naming it turns a vibe into a teachable moment.
- Turn the cheap detections on. Set
max-args,max-statements, and a complexity threshold in your linter so the obvious smells get flagged automatically and review can focus on the judgment calls. - Pick the one function your team is most afraid to touch. Identify which smell makes it scary, then extract the single most testable piece. You don't have to fix all of it today.
Key takeaways
- Code smells aren't bugs — they're design problems that make changes expensive and bugs likely. The value is recognizing them in review.
- Each smell has a recognition tell: scrolling (long method), names that don't rhyme (god class), fixing a bug twice (duplication), a method reaching into another object (feature envy), parameters that travel together (data clumps).
- Automate the mechanical detections; spend human review on the judgment calls.
- Every smell has an opposite. Refactor toward cheaper change, not toward a clean linter report.
Your next step
Open your most-changed file and read just the signatures — function names, parameter lists, class method lists. You'll spot more smells from the shape than from the logic. The one that makes you wince is where your next refactor pays for itself.
Frequently asked questions
What is a code smell?
A surface pattern that signals a deeper design problem — a long method, a god class, duplicated logic. Unlike a bug, a smell doesn't fail; it quietly makes the code expensive to change and bugs more likely. The practical skill is recognizing smells in review and acting on the ones that matter.
How is a code smell different from a bug?
A bug produces wrong behavior now. A smell produces wrong economics later — slower changes, riskier edits, more bugs over time. Code with smells often works perfectly; it just costs more every time someone touches it.
What are the most common code smells to watch for?
Long methods (more than one reason to change), large/god classes (method names from different domains), duplicate code (a fix has to be applied twice), feature envy (a method reaching into another object's data), and data clumps (the same parameters always traveling together). Each has a distinct recognition tell.
Can you have too little duplication?
Yes. Deduplicating two pieces of code that merely look alike but change for different reasons couples them — now a change meant for one breaks the other. The smell is shared logic that will change together, not coincidental resemblance. When in doubt, wait for the third occurrence before abstracting.
Should I refactor every smell I find?
No. A smell is a prompt to look, not a command to act. Over-refactoring — too many tiny functions, needless indirection, premature abstraction — is its own smell. The only test that matters is whether the next person changes the code faster, not whether the linter is happy.
#code-smells#refactoring#software-quality#technical-debt#python#best-practices#maintainability
Ruchit Suthar
15+ years scaling teams from startup to enterprise. 1,000+ technical interviews, 25+ engineers led. Real patterns, zero theory.
