Feature/refactor code smells - #97
Open
Rokib1605 wants to merge 2 commits into
Open
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This pull request applies five structural refactoring improvements to ExpiringMap.java to improve readability, reduce complexity, and better satisfy SOLID design principles. All 35 existing tests pass unchanged, and no public API signatures were modified.
Problems Identified
God Class: ExpiringMap.java at 1,427 lines handles six unrelated responsibilities simultaneously, violating SRP
Long Methods: put() and related methods exceed 40 lines mixing multiple operations
Deep Nesting: get(), scheduleEntry() contain 3-4 levels of nested conditionals
Duplicate Lock Pattern: Identical try/finally lock boilerplate repeated across 15+ methods, violating DRY
Inner Class Coupling: ExpiringEntry defined inside ExpiringMap prevents independent testing and reuse
Ambiguous Names: expirationNanos, listenerService, expirer do not clearly communicate intent
Refactoring Techniques Applied
Extract Method: put() decomposed into putInternal(), createAndScheduleEntry(), updateExistingEntry()
Guard Clauses: Nested conditionals in get() replaced with early returns
Rename Variable: Five ambiguous identifiers renamed to clearly express intent
Extract Helper: Duplicate lock pattern consolidated into withWriteLock() functional helper
Extract Class: ExpiringEntry moved to its own package-private file
SOLID Principles Improved
SRP: Each extracted method now has one clear responsibility
OCP: withWriteLock() helper allows locking strategy to change without touching every method
DRY: Lock boilerplate defined once instead of 15 times
Testing Evidence
Before refactoring: Tests run: 35, Failures: 0, Errors: 0 — BUILD SUCCESS
After refactoring: Tests run: 35, Failures: 0, Errors: 0 — BUILD SUCCESS
All public API signatures remain identical. No test files were modified.
Impact on Maintainability
Future contributors can now understand put() in under 10 seconds instead of tracing 40 lines. The extracted ExpiringEntry class can be tested and documented independently. The withWriteLock() helper means any future locking strategy change requires editing one method instead of fifteen.