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

Make Global a singleton instead of static class #1587

Merged
merged 3 commits into from Jun 18, 2019

Conversation

Projects
None yet
4 participants
@NicolasDorier
Copy link
Contributor

commented Jun 18, 2019

I am trying to refactor the code to limit the use of global singleton/god object which is an anti pattern.

By series of refactoring I will discover many bug along the way as the compiler will detect when I am accessing fields when I should not (such bug like this one should be discoverable by the compiler if there was not this huge Global)

Another cool thing, is that we would be able to run the tests in parallel, or easily create several instance of wasabi wallet in test which mix with one another in tests.

This PR is automatic Global => Global.Instance this will allow me to refactor the rest in an incremental way.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Another cool thing, is that we would be able to run the tests in parallel, or easily create several instance of wasabi wallet in test which mix with one another in tests.

We do that already. We create multiple WalletService-es.

LGTM, but can you make the Backend and the Test project's Global to be like this, too? So to be consistent with the pattern.

@lontivero @molnard opinions?

@nopara73 nopara73 requested review from molnard and lontivero Jun 18, 2019

@Rmartin1104

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Operating System: windows 10

  • pass Wasabi GUI exit test
  • pass Filter downloading tests
@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

I am trying to keep PRs as small as possible so I can move on the next step to diminish the reliance on the singleton.

If you are not scared by size of PR I can do that for other globals as well yes.

@lontivero
Copy link
Collaborator

left a comment

Everything looks good to me.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

We do that already. We create multiple WalletService-es.

Yeah, but I think you should create mutiple Global. You will have a higher test coverage like this, the tests will look like more the real running code and some exception like I got in https://github.com/zkSNACKs/WalletWasabi/pull/1585/files#r294717275 would not be able to happen.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

It might even reach the stage where Wasabi can be exposed as a library!

nopara73 added some commits Jun 18, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

The Global is not referenced from the test project. The test project only references the library project (WalletWasabi). By extension Wasabi could already be used as a library, but we want to break user space so we don't give out nuget.

I implemented the test project's global and the backend's global.instance pattern.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Tests pass.

@nopara73 nopara73 merged commit 3dfdf76 into zkSNACKs:master Jun 18, 2019

3 of 4 checks passed

Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Wasabi.Linux #20190618.11 succeeded
Details
Wasabi.Osx #20190618.11 succeeded
Details

@NicolasDorier NicolasDorier deleted the NicolasDorier:refactor/global-singleton branch Jun 19, 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.