-
-
Notifications
You must be signed in to change notification settings - Fork 83
* Remove redundant local variables #112
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
Conversation
WalkthroughThe PR simplifies code in four utility classes by eliminating intermediate local variables and consolidating variable initializations. Changes include removing unnecessary null assignments and directly returning computed values, without altering functionality. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/spring/property/SpringValueDefinitionProcessor.java (1)
75-77: DirectcomputeIfAbsentuse is clean and keeps behavior unchangedUsing
beanName2SpringValueDefinitions.computeIfAbsent(registry, k -> LinkedListMultimap.create())is equivalent to the previous get‑then‑put pattern and fits the concurrent map usage. Optionally, you could also call this helper fromprocessPropertyValuesinstead of manually checkingcontainsKeyandput, to avoid duplication, but that’s not required for this PR’s goal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/spring/property/SpringValueDefinitionProcessor.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/foundation/internals/NetworkInterfaceManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-10-19T12:48:00.553Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RepositoryChangeListener.java:32-38
Timestamp: 2024-10-19T12:48:00.553Z
Learning: In the Apollo Java client, the class `PureApolloConfig` extends `DefaultConfig`, which implements the `onRepositoryChange(String appId, String namespace, Properties newProperties)` method from the `RepositoryChangeListener` interface. Therefore, `PureApolloConfig` doesn't need its own implementation of this method.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
📚 Learning: 2024-10-24T02:13:17.143Z
Learnt from: TerryLam2010
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java:141-144
Timestamp: 2024-10-24T02:13:17.143Z
Learning: In `apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java`, when implementing the `ConfigFactory`, the method `createConfigFile(String appId, String namespace, ConfigFileFormat configFileFormat)` can return `null` as per the author's original design.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
📚 Learning: 2024-10-24T02:09:10.676Z
Learnt from: TerryLam2010
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java:68-69
Timestamp: 2024-10-24T02:09:10.676Z
Learning: In `apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java`, it's acceptable for `ConfigUtil.getAppId()` to return `null`, and the calling code will handle `null` return values appropriately.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
📚 Learning: 2024-10-19T12:52:36.706Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/test/java/com/ctrip/framework/apollo/spring/config/CachedCompositePropertySourceTest.java:85-85
Timestamp: 2024-10-19T12:52:36.706Z
Learning: In the Java class `ConfigChangeEvent` (`apollo-client/src/main/java/com/ctrip/framework/apollo/model/ConfigChangeEvent.java`), the constructor has been updated to accept three parameters: `String appId`, `String namespace`, and `Map<String, ConfigChange> changes`.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
📚 Learning: 2024-11-20T06:14:47.515Z
Learnt from: TerryLam2010
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/main/java/com/ctrip/framework/apollo/spring/spi/DefaultApolloConfigRegistrarHelper.java:65-69
Timestamp: 2024-11-20T06:14:47.515Z
Learning: In `DefaultApolloConfigRegistrarHelper.java`, it's acceptable to set the system property `apollo.accesskey.{appId}.secret` with a `null` value because downstream code handles `null` secrets appropriately.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
📚 Learning: 2024-10-26T12:58:36.781Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactoryManager.java:45-46
Timestamp: 2024-10-26T12:58:36.781Z
Learning: A interface `ConfigFactoryManager` declara o método `getFactory(String appId, String namespace)`.
Applied to files:
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistry.java (1)
61-64: Simplified factory lookup is semantically identicalReturning
m_instances.get(appId, namespace)directly removes dead local state without changing null/lookup behavior. All good here.apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (1)
71-88: Removing redundantnullinitialization is safe and clearer
configRepositoryis now declared without= null, and all branches still assign it before use, so there’s no change in behavior and no risk of using it uninitialized. This nicely matches the PR’s intent to trim redundant locals.apollo-core/src/main/java/com/ctrip/framework/foundation/internals/NetworkInterfaceManager.java (1)
95-103: Property lookup refactor keeps semantics identicalInitializing
valuedirectly withSystem.getProperty(name)and then falling back toSystem.getenv(name)preserves the original resolution order while dropping unnecessary boilerplate. No behavioral impact.
What's the purpose of this PR
Remove redundant local variables
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.