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

Remove redundant check #1925

Merged
merged 2 commits into from Aug 1, 2019

Conversation

@yahiheb
Copy link
Collaborator

commented Jul 16, 2019

No description provided.

yahiheb and others added 2 commits Jul 16, 2019

@nopara73 nopara73 merged commit d8b1587 into zkSNACKs:master Aug 1, 2019

1 check was pending

CodeFactor Reviewing...
Details
@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

@nopara73 I think your change is wrong.
Let's say if (needsPinWalletInfo is null) is false (line 463) therefore keyManager.HardwareWalletInfo will become not null, and let's say the next three if statements are all false, then we will get to return (false, "Could not find hardware wallet. Make sure it's plugged in and you're logged in with your PIN."); but this should be returned if keyManager.HardwareWalletInfo is null.

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

@nopara73 my commit could have been kept though.

@yahiheb yahiheb deleted the yahiheb:redundant-check-SendTabViewModel branch Aug 1, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

@yahiheb The original says if HardwareWalletInfo is still null, then it didn't find anything. My change was wrong, because the HardwareWalletInfo could had been changed before nullcheck.
In your change it's the opposite: what if someone leaves HardwareWalletInfo null under the if (needsPinWalletInfo != null) brackets? So the original is the cleanest version.

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 2, 2019

@nopara73 Correct me if I am wrong.
If (needsPinWalletInfo != null) is false then we are sure keyManager.HardwareWalletInfo is null because it didn't change, therefore we can return (false, "Could not find hardware.... in the else that I add without checking again.
If (needsPinWalletInfo != null) is true but we get to its closing bracket then we are sure keyManager.HardwareWalletInfo is not null because it was set to needsPinWalletInfo value that is not null.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

The (needsPinWalletInfo != null) is true case is not future proofed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.