-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
optimize: simplify if else judgment #3939
Conversation
can the DatabaseTransactionStoreManager be optimize like this? |
|
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.
LGTM
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.
Can you put some change log in change module like other PR?
ok,i finished it |
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.
LGTM
Codecov Report
@@ Coverage Diff @@
## develop #3939 +/- ##
=============================================
- Coverage 49.59% 40.35% -9.24%
+ Complexity 3730 3072 -658
=============================================
Files 693 685 -8
Lines 23402 23203 -199
Branches 2899 2864 -35
=============================================
- Hits 11606 9364 -2242
- Misses 10631 12969 +2338
+ Partials 1165 870 -295
|
if (globalMap.containsKey(logOperation) || branchMap.containsKey(logOperation)) { | ||
return globalMap.containsKey(logOperation) ? | ||
globalMap.get(logOperation).apply(SessionConverter.convertGlobalTransactionDO(session)) : | ||
branchMap.get(logOperation).apply(SessionConverter.convertBranchTransactionDO(session)); |
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.
In my opinion,there is no need to invoke globalMap.containsKey(logOperation)
again.it is better to break the if
statement into two if
statements.
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.
+1
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.
为何不把db那块实现一并修改?
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.
LGTM
This pull request introduces 2 alerts when merging 9e03023 into 129c2f3 - view on LGTM.com new alerts:
|
Ⅰ. Describe what this PR did
use two ImmutableMap instead of lots of if else judge, and the code is more readability,more convenient to modify.
Ⅱ. Does this pull request fix one issue?
切换develop分支 以及将map并入构造器 旧pr #3932
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews