optimize: clarify if conditions #6442
Conversation
YeonCheolGit
force-pushed
the
yeoncheol/2.x
branch
from
da7b089 to
033585a
Compare
YeonCheolGit
changed the title
|
good |
|
cc. @funky-eyes |
|
|
||
| // The entire undo process should run in a local transaction. | ||
| if (originalAutoCommit = conn.getAutoCommit()) { | ||
| if (conn.getAutoCommit()) { |
There was a problem hiding this comment.
Sorry, I don't understand the purpose of doing this. Here, conn.getAutoCommit() is used twice.
There was a problem hiding this comment.
@slievrly Thank you for the review.
I intended to divdie assign to variable and check whether is true or not in If condtion for clarify. conn.getAutoCommit() returns only state so i assumed will be okay for using twice.
What about these alternatively?
originalAutoCommit = conn.getAutoCommit();
if (originalAutoCommit)
or
final boolean autoCommitState = conn.getAutoCommit();
originalAutoCommit = autoCommitState
if (autoCommitState)
There was a problem hiding this comment.
I prefer the first expression for its simplicity and fidelity to the original meaning.
There was a problem hiding this comment.
Thanks for your review!
I've addressed your comments and pushed a new commit with the changes 😄
slievrly
changed the title
|
Additionally, remember to update the change log. This PR falls under the category of optimization. |
This if condition seems to have three pieces of logic: 1. Check if conn.getAutoCommit() is true. 2. Assign conn.getAutoCommit() to originalAutoCommit. 3. If conn.getAutoCommit() is true, then set conn.setAutoCommit(true). However, the if condition is assigning originalAutoCommit from conn.getAutoCommit() and checking whether conn.getAutoCommit() is true or false simultaneously. clarify and divide the logic
This if condition seems to have three pieces of logic: 1. Acquire a distributed lock with a key. 2. Assign the acquired distributed lock to the 'lock' variable. 3. If the lock is successfully acquired, then make a function call. However, the if condition is doing two jobs simultaneously, which makes it unclear. divide and clarify the logic.
YeonCheolGit
force-pushed
the
yeoncheol/2.x
branch
from
033585a to
ec35f20
Compare
I changed commit message from 'refactor' to 'optimize'. Thanks for the review :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #6442 +/- ##
============================================
+ Coverage 50.26% 50.85% +0.59%
- Complexity 5242 5286 +44
============================================
Files 942 942
Lines 33270 33273 +3
Branches 4030 4030
============================================
+ Hits 16722 16922 +200
+ Misses 14929 14685 -244
- Partials 1619 1666 +47
|
|
@YeonCheolGit Hi,Please register your PR in those two files : |
Thank you for letting me know. I added my PR in these two files. |
|
You should update the changes file in this PR without creating a separate PR. |
Oh i misunderstanded. Add commit in this PR. |
| @@ -115,6 +115,11 @@ | |||
| - [[#6387](https://github.com/apache/incubator-seata/pull/6387)] 优化tcc使用兼容 | |||
| - [[#6402](https://github.com/apache/incubator-seata/pull/6402)] 优化rm-datasource向下兼容 | |||
| - [[#6419](https://github.com/apache/incubator-seata/pull/6419)] 优化integration-tx-api向下兼容 | |||
| - [[#6442](https://github.com/apache/incubator-seata/pull/6442)] 阐明 if | |||
There was a problem hiding this comment.
The change log look there are some problems.
Ⅰ. Describe what this PR did
(please see each commit)
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
It has already tese cases.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews