megaease / easemesh

A service mesh implementation for connecting, control, and observe services in spring-cloud.
https://megaease.com/easemesh
Apache License 2.0
508 stars 61 forks source link

Refactor the code to improve code quality and More concise code logic #129

Closed diannaowa closed 1 year ago

diannaowa commented 2 years ago

Remove unreachable code and for loop. Refactor the code to improve code quality and More concise code logic.

codecov-commenter commented 2 years ago

Codecov Report

Merging #129 (aaba90b) into main (aafdcb6) will decrease coverage by 0.31%. The diff coverage is 93.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
- Coverage   76.32%   76.00%   -0.32%     
==========================================
  Files          77       77              
  Lines        6235     6123     -112     
==========================================
- Hits         4759     4654     -105     
+ Misses       1220     1213       -7     
  Partials      256      256              
Impacted Files Coverage Δ
emctl/cmd/client/command/apply/applier.go 93.90% <93.75%> (+0.04%) :arrow_up:
emctl/cmd/client/command/apply/apply.go 91.66% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update aafdcb6...aaba90b. Read the comment docs.

diannaowa commented 2 years ago

Thanks for your comments. Yes, it is a retry-able mechanism, but there are some logical errors in switch statement. The condition in the if statement is always false at the case meshclient.IsConflictError and meshclient.IsNotFoundError of the switch, because the patch function wont return ConflictError and create function wont return NotFoundError forever.

And, There is no need for a retry mechanism in the apply method. When an error occurs, the method can be exited directly, and the user can re-execute the apply command. In most cases the create function is executed successfully.

I think we need to refactor the implementation logic here, let's find an implementation that meets the needs and is concise. This PR is an implementation I gave, please take a look.

zhao-kun commented 2 years ago

Thanks for your comments. Yes, it is a retry-able mechanism, but there are some logical errors in switch statement. The condition in the if statement is always false at the case meshclient.IsConflictError and meshclient.IsNotFoundError of the switch, because the patch function wont return ConflictError and create function wont return NotFoundError forever.

And, There is no need for a retry mechanism in the apply method. When an error occurs, the method can be exited directly, and the user can re-execute the apply command. In most cases the create function is executed successfully.

I think we need to refactor the implementation logic here, let's find an implementation that meets the needs and is concise. This PR is an implementation I gave, please take a look.

It hasn't errors in the logic, you could carefully check it. Although the Create function won't return NotFoundError, the Patch function can and vice versa. This is a simple tiny state machine, each branch handles the error it concerns. It's a common programming paradigm in the old C style.