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

Set statuses in subscribe instead of in the setter #1424

Merged
merged 2 commits into from May 11, 2019

Conversation

Projects
None yet
3 participants
@nopara73
Copy link
Collaborator

commented May 9, 2019

I think it's not controversial. Can I get an approve from someone?

  • Renamed SetWalletStates to TrySetWalletStates and encapsulated with a try catch.
  • Moved the setter logic from IsBusy to subscription.

@nopara73 nopara73 requested review from lontivero and molnard May 9, 2019

@@ -569,9 +570,8 @@ public async Task LoadWalletAsync()
}
finally
{
IsBusy = false;
MainWindowViewModel.Instance.StatusBar.RemoveStatus(loadingStatusText);

This comment has been minimized.

Copy link
@molnard

molnard May 10, 2019

Contributor

Move this to the last line of try block above? I don't like the feeling to have an exception here. To much logic in RemoveStatus.

This comment has been minimized.

Copy link
@nopara73

nopara73 May 10, 2019

Author Collaborator

There can be no exceptions in RemoveStatus. But I don't understand what you are suggesting. Can you make a PR?

This comment has been minimized.

Copy link
@molnard

molnard May 10, 2019

Contributor

If it cannot than it is fine. My suggestion was to move the following code:
MainWindowViewModel.Instance.StatusBar.RemoveStatus(loadingStatusText);
below line 568.

This comment has been minimized.

Copy link
@nopara73

nopara73 May 11, 2019

Author Collaborator

You have to put the RemoveStatus in a finally that has been opened with a try and immediately followed by a AddStatus, so putting it into that catch block makes no sense.

However I made the add-remove, tryadd-tryremove, so you'll feel safer.

}
}

private void SetWalletStates()
private bool TrySetWalletStates()

This comment has been minimized.

Copy link
@lontivero

lontivero May 10, 2019

Contributor

The returned value is never used. It could return void.

This comment has been minimized.

Copy link
@nopara73

nopara73 May 11, 2019

Author Collaborator

FooList.TryRemove(bar);

If a TrySomething can return a useful bool, then it should, even if it's not used. Not returning is an antipattern. I'm fine with this antipattern when it's out of convenience, if the function is so complex, returning a bool would be a pain and that bool would be never used, but intentionally doing so, when the correct pattern is just a few lines away is problematic.

@nopara73 nopara73 merged commit 4385a71 into zkSNACKs:master May 11, 2019

1 of 2 checks passed

Wasabi.Linux queued
Details
CodeFactor No issues found.
Details

@nopara73 nopara73 deleted the nopara73:subscribe branch May 11, 2019

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