Skip to content
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

Refactor of TryAdd method #11687

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Conversation

M1nd3r
Copy link
Contributor

@M1nd3r M1nd3r commented Oct 13, 2023

Hi, I am new on this project and I am not sure about the "right" process. I just saw a multiple-level nested if statements in a code I was trying to read. Thus, I have decided to refactor the code to improve readability (extract one method into four).

If extraction of methods is less preferable to nested if blocks, feel free to disregard/close this pull request.

Thanks for any feedback

@lontivero
Copy link
Collaborator

lontivero commented Oct 13, 2023

Improving the quality of the code is always welcome and avoiding nesting is something that generally speaking improves the quality of the code by making it more readable. Readability is the goal, removing nesting is just one way to achieve that.

In this case the elimination of the nested blocks doesn't improves the readability but it make it worse instead. It is impossible to me at least to understand what the method does after this refactoring because of the empty semantic of
InitializeSmartCoinDataNoLock and UpdateCoinMappingsNoLock. It is for that that in order to understand what is going on I am forced to jump to those methods and then jump back to continue reading the method code.

@M1nd3r
Copy link
Contributor Author

M1nd3r commented Oct 13, 2023

Ok, I see what you mean @lontivero . I dialed the extraction down - the level of nesting is still reduced by two compared to the original.

Do you think this approach is ok?

@lontivero
Copy link
Collaborator

I think so, yes.

@M1nd3r M1nd3r marked this pull request as ready for review October 13, 2023 23:31
yahiheb
yahiheb previously approved these changes Oct 13, 2023
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kiminuo
kiminuo previously approved these changes Oct 15, 2023
Copy link
Collaborator

@kiminuo kiminuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M1nd3r M1nd3r dismissed stale reviews from kiminuo and yahiheb via d4d4ee9 October 15, 2023 20:33
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kiminuo kiminuo merged commit 36a4654 into WalletWasabi:master Oct 16, 2023
7 checks passed
@kiminuo
Copy link
Collaborator

kiminuo commented Oct 16, 2023

@M1nd3r Thanks

@M1nd3r M1nd3r deleted the M1nd3r/refactorTryAdd branch October 16, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants