Skip to content

Code Quality Improvement #394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

temidireDimowo
Copy link

No description provided.

@Tembrel
Copy link
Contributor

Tembrel commented Mar 30, 2025

With one exception, these changes seem gratuitous to me, and in one case, wrong.

  • Explanatory variables in RandomDelay. This is a particularly common idiom that I wouldn't have thought needed explaining. And I don't see why the formal parameters needed to be renamed (delayMin -> minDelay), especially when one of the introduced variables (delayRange) uses the pre-renaming pattern. And there's no need for a comment describing the refactoring; no one reading the code needs to know that it was refactored from something else.
  • Improve variable naming in FutureLinkedList. The names head, tail, and node are common and idiomatic. They have the virture of being short and all of the same length. There's no need to embed type information in the field name.
  • 👍Decompose conditional in transitionTo method of CircuitBreakerImpl. This is a real improvement, not because of the refactoring into a separate private method (although there's nothing wrong with that), but because it uses the value of currentState assigned in line 163 when comparing to newState in line 164, instead of calling getState() again.
  • Move Method acquirePermit() from interface to implementation. I think it helps to have default implementations when they are short and can help document expectations. And since this PR doesn't remove the default implementation, this refactoring actually raises the question: "Why are there two identical implementations?"
  • 🚫 Pull-Up Field exceptionsChecked field from FailurePolicyConfig. I don't see the point of this. It introduces a field in PolicyConfig that isn't used there and is hidden by the one in FailurePolicyConfig.
  • 🆗Extract Class. This is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants