-
-
Notifications
You must be signed in to change notification settings - Fork 294
Description
Description
After analyzing the current implementation of the Library Management System project, several code smells were identified that negatively affect maintainability, extensibility, and readability of the system. The most relevant issues are related to high coupling, low cohesion, and concentration of logic in certain classes.
The Book class currently manages multiple responsibilities, including hold request operations and borrower interactions. This results in frequent access to internal data of other classes (HoldRequestOperations, Borrower), breaking encapsulation and increasing dependency between components. Additionally, the User class hierarchy may introduce unnecessary complexity, and the static ID generation approach creates thread-safety and testing issues.
Identified Problems
The following code smells were identified:
- Message Chains: The Book class accesses nested structures like
holdRequestsOperations.holdRequests.size(), creating tight coupling and violating the Law of Demeter. - Feature Envy: Methods in Book extensively manipulate Borrower's data instead of their own, indicating misplaced responsibilities.
- Middle Man: HoldRequestOperations may act as an unnecessary intermediary if it only delegates without adding business logic.
- Temporary Field: The static
currentIdNumbervariable creates thread-safety issues and complicates testing. - Speculative Generality: The User class hierarchy may be overly complex for current requirements, adding unnecessary abstraction layers.
Proposed Refactorings
To address these issues, the following refactoring techniques are proposed and applied:
- Hide Delegate – Encapsulate access to HoldRequestOperations' internal collection by adding delegate methods (
getHoldRequestsCount(),getHoldRequestAt()). - Move Method – Transfer book return logic from Book to Borrower where the data resides, improving cohesion.
- Remove Middle Man – Evaluate if HoldRequestOperations adds value or can be simplified/removed to reduce unnecessary indirection.
- Extract Class – Create a dedicated BookIdGenerator class for thread-safe ID generation with reset capability for testing.
- Collapse Hierarchy – Simplify the User inheritance structure by using a single LibraryUser class with role enums (BORROWER, LIBRARIAN, CLERK) instead of multiple subclasses.
Benefits
- Reduced coupling between classes
- Improved cohesion and clearer responsibilities
- Easier maintenance and extensibility
- Cleaner and more readable codebase
- Better alignment with object-oriented design principles
- Enhanced testability and thread-safety
- Simplified architecture with fewer classes
These refactorings aim to improve code quality while preserving the original functionality of the system. Feedback and suggestions are welcome.
References: