Code Refactoring Set 2: Move Method, Extract Class, Replace Conditional with Polymorphism #786
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ce class Refactored to fix feature envy code smell. The _get_abilities method was accessing resource object members more than its own class members, indicating it belonged in the Resource class.
…rvice Refactored to fix multifaceted abstraction code smell. The BaseEgressService was handling both egress lifecycle management and LiveKit API communication, indicating it had multiple responsibilities that should be separated into distinct classes.
…conditional logic in NotificationService with strategy pattern Replaced if/elif conditional checks based on recording.mode with polymorphic strategy classes. Created RecordingNotificationStrategy base class with TranscriptNotificationStrategy and ScreenRecordingNotificationStrategy subclasses. Each strategy implements its own notify method, achieving proper implementation through polymorphism instead of conditional branching.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactorings Applied:
1. Move Method - models.py
Problem: Feature envy code smell - the _get_abilities method in BaseAccess class was accessing resource object members (accesses, roles, filters) more than its own class members, indicating it didn't belong in that class
Solution: Moved the abilities computation logic from BaseAccess to Resource class, creating a new get_access_abilities(access, user) method. The original _get_abilities in BaseAccess now delegates to this method to maintain the interface
Benefit: Eliminated feature envy by placing the method in the class that owns the data it primarily uses. Improved cohesion by keeping resource access logic within the Resource class, and reduced coupling by making the relationship more explicit
2. Extract Class - services.py
Problem: Multifaceted abstraction code smell - BaseEgressService was handling two distinct responsibilities: egress lifecycle management (start/stop operations, file path construction) AND LiveKit API communication (client creation, request execution, error handling)
Solution: Extracted LiveKitAPIClient as a separate class responsible for all LiveKit API communication:
Handles LiveKit client creation and configuration
Executes API requests with proper error handling (TwirpError, generic exceptions)
Manages connection lifecycle (async/await, cleanup)
BaseEgressService now uses LiveKitAPIClient via composition (self._api_client)
Benefit: Separated concerns into distinct classes with single responsibilities. The API communication layer is now isolated and can be tested/modified independently from egress business logic. Improved modularity and adherence to Single Responsibility Principle
3. Replace Conditional with Polymorphism - notification.py
Problem: Type-based conditional logic in notify_external_services method that performed different actions based on recording.mode property using if/elif statements
Solution: Replaced conditional branching with Strategy pattern:
Created RecordingNotificationStrategy base class defining the notify() interface
Implemented TranscriptNotificationStrategy subclass for transcript recordings (delegates to summary service)
Implemented ScreenRecordingNotificationStrategy subclass for screen recordings (delegates to email notification)
Strategy selection via dictionary lookup: _notification_strategies maps RecordingModeChoices to strategy classes
Each strategy implements polymorphic notify() method
Benefit: Achieved polymorphic behavior through strategy pattern, eliminating conditional branching based on type. Makes it easier to add new recording modes (just add a new strategy class). Follows Open/Closed Principle - open for extension, closed for modification
Similar to the Set-1 PR. No functionality was changed. The system was run end-to-end after changes and was successfully executed.
Please reach out regarding any concerns and I'll be happy to answer.