-
Notifications
You must be signed in to change notification settings - Fork 11.5k
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
[ISSUE #5415]fix duplicate records #5416
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5416 +/- ##
=============================================
- Coverage 43.17% 43.13% -0.04%
+ Complexity 8046 8038 -8
=============================================
Files 1020 1020
Lines 71892 71887 -5
Branches 9519 9519
=============================================
- Hits 31039 31011 -28
- Misses 36946 36960 +14
- Partials 3907 3916 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
if ((opQueue = opQueueMap.get(queueId)) == null) {
You write this way although it looks cleaner, but it is not easy to read the code
MessageQueue opQueue; | ||
if (opQueueMap.containsKey(queueId)) { | ||
opQueue = opQueueMap.get(queueId); | ||
} else { | ||
if ((opQueue = opQueueMap.get(queueId)) == null) { |
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.
Assigning opQueue
at first may be better than initializing it in if
condition.
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.
Ok, I think this can reduce one query, although the time complexity is only O(1) and I don't think it's a good practice to judge first and then get it. If the opQueueMap has a removal operation, there will be problems.
issue