refactoring_after.py

Download
python 488 lines 14.9 KB
  1"""
  2GOOD CODE EXAMPLE - After Refactoring
  3
  4This code demonstrates clean code principles:
  51. Single Responsibility Principle
  62. Named constants instead of magic numbers
  73. Clear, descriptive naming
  84. Small, focused functions
  95. Proper error handling
 106. No deep nesting (early returns)
 117. Separation of concerns
 128. Dependency injection
 139. Type hints for clarity
 14"""
 15
 16from typing import List, Dict, Optional, Protocol
 17from dataclasses import dataclass
 18from enum import Enum
 19
 20
 21# ✅ Named constants instead of magic numbers
 22class EmployeeConstants:
 23    """Constants for employee management"""
 24    MINIMUM_AGE = 18
 25    MINIMUM_NAME_LENGTH = 2
 26    PREMIUM_SALARY_THRESHOLD = 150_000
 27    BONUS_MULTIPLIER = 1.2
 28
 29    # Bonus rate thresholds
 30    SALARY_TIER_1 = 50_000
 31    SALARY_TIER_2 = 100_000
 32    SALARY_TIER_3 = 150_000
 33
 34    BONUS_RATE_TIER_1 = 0.10
 35    BONUS_RATE_TIER_2 = 0.15
 36    BONUS_RATE_TIER_3 = 0.20
 37    BONUS_RATE_PREMIUM = 0.25
 38    BONUS_PREMIUM_EXTRA = 5_000
 39
 40    TEAM_BONUS_THRESHOLD = 10
 41    TEAM_BONUS_MULTIPLIER = 1.10
 42    BULK_DISCOUNT_THRESHOLD = 100_000
 43    BULK_DISCOUNT_RATE = 0.30
 44
 45
 46# ✅ Enum for employee tier (better than boolean or magic strings)
 47class EmployeeTier(Enum):
 48    STANDARD = "standard"
 49    PREMIUM = "premium"
 50
 51
 52# ✅ Data class with type hints - clear structure
 53@dataclass
 54class Employee:
 55    """
 56    Represents an employee with all relevant information.
 57    Immutable after creation (frozen=True would make it fully immutable).
 58    """
 59    name: str
 60    email: str
 61    age: int
 62    salary: float
 63    tier: EmployeeTier = EmployeeTier.STANDARD
 64
 65    def calculate_bonus(self) -> float:
 66        """Calculate employee bonus based on salary tier"""
 67        if self.salary < EmployeeConstants.SALARY_TIER_1:
 68            return self.salary * EmployeeConstants.BONUS_RATE_TIER_1
 69
 70        elif self.salary < EmployeeConstants.SALARY_TIER_2:
 71            return self.salary * EmployeeConstants.BONUS_RATE_TIER_2
 72
 73        elif self.salary < EmployeeConstants.SALARY_TIER_3:
 74            return self.salary * EmployeeConstants.BONUS_RATE_TIER_3
 75
 76        else:
 77            base_bonus = self.salary * EmployeeConstants.BONUS_RATE_PREMIUM
 78            return base_bonus + EmployeeConstants.BONUS_PREMIUM_EXTRA
 79
 80
 81# ✅ Custom exceptions for specific error cases
 82class ValidationError(Exception):
 83    """Raised when validation fails"""
 84    pass
 85
 86
 87class DuplicateEmailError(ValidationError):
 88    """Raised when trying to add duplicate email"""
 89    pass
 90
 91
 92class EmployeeNotFoundError(Exception):
 93    """Raised when employee doesn't exist"""
 94    pass
 95
 96
 97# ✅ Single Responsibility: Validator class only validates
 98class EmployeeValidator:
 99    """Validates employee data before creation"""
100
101    @staticmethod
102    def validate_age(age: int) -> None:
103        """Validate employee age"""
104        if age < EmployeeConstants.MINIMUM_AGE:
105            raise ValidationError(
106                f"Employee must be at least {EmployeeConstants.MINIMUM_AGE} years old"
107            )
108
109    @staticmethod
110    def validate_name(name: str) -> None:
111        """Validate employee name"""
112        if len(name) < EmployeeConstants.MINIMUM_NAME_LENGTH:
113            raise ValidationError(
114                f"Name must be at least {EmployeeConstants.MINIMUM_NAME_LENGTH} characters"
115            )
116
117    @staticmethod
118    def validate_email(email: str) -> None:
119        """Validate email format (basic check)"""
120        if "@" not in email or "." not in email:
121            raise ValidationError("Invalid email format")
122
123    @staticmethod
124    def validate_salary(salary: float) -> None:
125        """Validate salary is positive"""
126        if salary < 0:
127            raise ValidationError("Salary cannot be negative")
128
129    @classmethod
130    def validate_employee(cls, name: str, email: str, age: int, salary: float) -> None:
131        """Validate all employee fields"""
132        cls.validate_name(name)
133        cls.validate_email(email)
134        cls.validate_age(age)
135        cls.validate_salary(salary)
136
137
138# ✅ Protocol for data storage (dependency inversion)
139class EmployeeRepository(Protocol):
140    """Interface for employee storage"""
141
142    def add(self, employee: Employee) -> None:
143        """Add employee to repository"""
144        ...
145
146    def find_by_email(self, email: str) -> Optional[Employee]:
147        """Find employee by email"""
148        ...
149
150    def get_all(self) -> List[Employee]:
151        """Get all employees"""
152        ...
153
154    def remove(self, email: str) -> None:
155        """Remove employee by email"""
156        ...
157
158    def update(self, email: str, **kwargs) -> None:
159        """Update employee fields"""
160        ...
161
162
163# ✅ Concrete implementation of repository
164class InMemoryEmployeeRepository:
165    """In-memory storage for employees"""
166
167    def __init__(self):
168        self._employees: List[Employee] = []
169
170    def add(self, employee: Employee) -> None:
171        """Add employee, checking for duplicates"""
172        if self.find_by_email(employee.email):
173            raise DuplicateEmailError(f"Email {employee.email} already exists")
174        self._employees.append(employee)
175
176    def find_by_email(self, email: str) -> Optional[Employee]:
177        """Find employee by email, return None if not found"""
178        for employee in self._employees:
179            if employee.email == email:
180                return employee
181        return None
182
183    def get_all(self) -> List[Employee]:
184        """Return copy of all employees (encapsulation)"""
185        return self._employees.copy()
186
187    def remove(self, email: str) -> None:
188        """Remove employee by email"""
189        employee = self.find_by_email(email)
190        if not employee:
191            raise EmployeeNotFoundError(f"Employee with email {email} not found")
192        self._employees.remove(employee)
193
194    def update(self, email: str, **kwargs) -> None:
195        """Update employee fields with validation"""
196        employee = self.find_by_email(email)
197        if not employee:
198            raise EmployeeNotFoundError(f"Employee with email {email} not found")
199
200        # Update allowed fields
201        if 'name' in kwargs:
202            EmployeeValidator.validate_name(kwargs['name'])
203            employee.name = kwargs['name']
204
205        if 'salary' in kwargs:
206            EmployeeValidator.validate_salary(kwargs['salary'])
207            employee.salary = kwargs['salary']
208
209        if 'age' in kwargs:
210            EmployeeValidator.validate_age(kwargs['age'])
211            employee.age = kwargs['age']
212
213
214# ✅ Service class with single responsibility
215class EmployeeManager:
216    """
217    Manages employee operations.
218    Uses dependency injection for flexibility.
219    """
220
221    def __init__(self, repository: EmployeeRepository):
222        self._repository = repository
223        self._employee_count = 0
224
225    def add_employee(
226        self,
227        name: str,
228        email: str,
229        age: int,
230        salary: float
231    ) -> Employee:
232        """
233        Add new employee with validation.
234        Clear name, clear purpose, proper error handling.
235        """
236        # Validate input (early return on error)
237        EmployeeValidator.validate_employee(name, email, age, salary)
238
239        # Determine tier based on salary
240        tier = (EmployeeTier.PREMIUM
241                if salary >= EmployeeConstants.PREMIUM_SALARY_THRESHOLD
242                else EmployeeTier.STANDARD)
243
244        # Create employee
245        employee = Employee(name, email, age, salary, tier)
246
247        # Add to repository (repository handles duplicate check)
248        self._repository.add(employee)
249        self._employee_count += 1
250
251        return employee
252
253    def get_employee(self, email: str) -> Optional[Employee]:
254        """Get employee by email, return None if not found"""
255        return self._repository.find_by_email(email)
256
257    def remove_employee(self, email: str) -> None:
258        """Remove employee by email"""
259        self._repository.remove(email)
260        self._employee_count -= 1
261
262    def update_employee(self, email: str, **kwargs) -> None:
263        """Update employee with validation"""
264        self._repository.update(email, **kwargs)
265
266    def get_employee_count(self) -> int:
267        """Get total number of employees"""
268        return self._employee_count
269
270    def calculate_total_bonuses(self) -> float:
271        """
272        Calculate total bonuses for all employees.
273        Clear method name, single responsibility.
274        """
275        employees = self._repository.get_all()
276
277        # Calculate base bonuses
278        total_bonuses = sum(emp.calculate_bonus() for emp in employees)
279
280        # Apply team bonus if threshold met
281        if self._should_apply_team_bonus(employees):
282            total_bonuses *= EmployeeConstants.TEAM_BONUS_MULTIPLIER
283
284        # Apply bulk discount if threshold met
285        if self._should_apply_bulk_discount(total_bonuses):
286            discount = total_bonuses * EmployeeConstants.BULK_DISCOUNT_RATE
287            total_bonuses -= discount
288
289        return total_bonuses
290
291    def _should_apply_team_bonus(self, employees: List[Employee]) -> bool:
292        """Check if team bonus should be applied (private helper)"""
293        return len(employees) > EmployeeConstants.TEAM_BONUS_THRESHOLD
294
295    def _should_apply_bulk_discount(self, total: float) -> bool:
296        """Check if bulk discount should be applied (private helper)"""
297        return total > EmployeeConstants.BULK_DISCOUNT_THRESHOLD
298
299
300# ✅ Pure functions with clear purpose
301def transform_data(data: List[int], threshold: int, high_multiplier: int,
302                   low_multiplier: int, offset: int) -> List[int]:
303    """
304    Transform data based on threshold.
305    Clear parameters instead of magic numbers.
306    """
307    return [
308        value * high_multiplier + offset if value > threshold
309        else value * low_multiplier
310        for value in data
311    ]
312
313
314# ✅ Proper error handling with context manager
315def read_and_process_csv(filename: str) -> List[Dict[str, str]]:
316    """
317    Read and process CSV file with proper error handling.
318    Uses context manager to ensure file is closed.
319    """
320    try:
321        with open(filename, 'r') as file:
322            data = file.read()
323
324        lines = data.strip().split('\n')
325        result = []
326
327        for line_num, line in enumerate(lines, start=1):
328            line = line.strip()
329
330            # Skip empty lines
331            if not line:
332                continue
333
334            parts = line.split(',')
335
336            # Validate format
337            if len(parts) < 3:
338                raise ValueError(
339                    f"Line {line_num}: Expected at least 3 fields, got {len(parts)}"
340                )
341
342            try:
343                result.append({
344                    'name': parts[0].strip(),
345                    'value': int(parts[1].strip()),
346                    'category': parts[2].strip()
347                })
348            except ValueError as e:
349                raise ValueError(f"Line {line_num}: Invalid number format") from e
350
351        return result
352
353    except FileNotFoundError:
354        raise FileNotFoundError(f"File not found: {filename}")
355    except PermissionError:
356        raise PermissionError(f"Permission denied: {filename}")
357
358
359# ✅ Split function instead of boolean parameter
360def get_active_employees(repository: EmployeeRepository) -> List[Employee]:
361    """Get only active employees (clear, single purpose)"""
362    # Implementation would filter active employees
363    return repository.get_all()
364
365
366def get_all_employees_including_inactive(repository: EmployeeRepository) -> List[Employee]:
367    """Get all employees including inactive (clear, single purpose)"""
368    return repository.get_all()
369
370
371# ✅ Clear function name and extracted calculation
372def calculate_weighted_sum(x: float, y: float, z: float) -> float:
373    """
374    Calculate weighted sum: ((x + y) * z - 10) / 2 * 1.5
375    Formula is now clear from implementation, not from comments.
376    """
377    sum_xy = x + y
378    multiplied = sum_xy * z
379    adjusted = (multiplied - 10) / 2
380    result = adjusted * 1.5
381    return result
382
383
384# ✅ Demonstration
385if __name__ == "__main__":
386    print("=" * 70)
387    print("CLEAN CODE DEMONSTRATION")
388    print("=" * 70)
389    print("\nThis code demonstrates:")
390    print("  ✓ Named constants (no magic numbers)")
391    print("  ✓ Clear, descriptive names")
392    print("  ✓ Single Responsibility Principle")
393    print("  ✓ Small, focused functions")
394    print("  ✓ Proper error handling")
395    print("  ✓ Type hints")
396    print("  ✓ Separation of concerns")
397    print("  ✓ Dependency injection")
398
399    print("\n" + "-" * 70)
400    print("Running clean code example:")
401    print("-" * 70)
402
403    # Create repository and manager
404    repository = InMemoryEmployeeRepository()
405    manager = EmployeeManager(repository)
406
407    # Add employees with clear error handling
408    try:
409        alice = manager.add_employee("Alice Smith", "alice@example.com", 25, 60_000)
410        print(f"✓ Added: {alice.name} ({alice.tier.value})")
411
412        bob = manager.add_employee("Bob Johnson", "bob@example.com", 30, 180_000)
413        print(f"✓ Added: {bob.name} ({bob.tier.value})")
414
415        # This will fail validation
416        try:
417            manager.add_employee("Charlie", "charlie@example.com", 15, 0)
418        except ValidationError as e:
419            print(f"✗ Validation failed: {e}")
420
421        # This will fail duplicate check
422        try:
423            manager.add_employee("Alice Clone", "alice@example.com", 30, 50_000)
424        except DuplicateEmailError as e:
425            print(f"✗ Duplicate email: {e}")
426
427    except Exception as e:
428        print(f"Error: {e}")
429
430    print(f"\nTotal employees: {manager.get_employee_count()}")
431    print(f"Total bonuses: ${manager.calculate_total_bonuses():,.2f}")
432
433    # Find employee
434    employee = manager.get_employee("alice@example.com")
435    if employee:
436        print(f"\nFound employee: {employee.name}")
437        print(f"  Email: {employee.email}")
438        print(f"  Salary: ${employee.salary:,.2f}")
439        print(f"  Bonus: ${employee.calculate_bonus():,.2f}")
440
441    print("\n" + "=" * 70)
442    print("IMPROVEMENTS FROM REFACTORING:")
443    print("=" * 70)
444    print("""
4451. READABILITY
446   ✓ Clear, descriptive names
447   ✓ Named constants explain meaning
448   ✓ Type hints document expected types
449   ✓ Single purpose per function
450
4512. MAINTAINABILITY
452   ✓ Changes localized to specific classes
453   ✓ No code duplication
454   ✓ Easy to modify without breaking
455   ✓ Constants in one place
456
4573. TESTABILITY
458   ✓ Small functions easy to test
459   ✓ Dependency injection allows mocking
460   ✓ Clear error cases
461   ✓ Pure functions where possible
462
4634. EXTENSIBILITY
464   ✓ Protocol/Interface for flexibility
465   ✓ Follows SOLID principles
466   ✓ Easy to add new features
467   ✓ Loose coupling
468
4695. RELIABILITY
470   ✓ Proper error handling
471   ✓ Input validation
472   ✓ Custom exceptions
473   ✓ Resource management (context managers)
474
475COMPARISON TO BEFORE:
476  Before: UsrMgr.p(n, e, a, s)  → What does this do?
477  After:  manager.add_employee(name, email, age, salary)  → Clear!
478
479  Before: Magic number 18  → Why 18?
480  After:  EmployeeConstants.MINIMUM_AGE  → Clear meaning!
481
482  Before: Deep nesting with duplicated logic
483  After:  Small functions with single purpose
484
485  Before: No error handling, crashes on bad input
486  After:  Validation and custom exceptions
487""")