nopara73 / HBitcoin

Privacy focused Bitcoin library on top of NBitcoin for .NET Core.
MIT License
26 stars 11 forks source link

Safe class small changes #2

Closed bokobza closed 7 years ago

bokobza commented 7 years ago

Made some small changes to the Safe class (mainly). In the first commit I just moved some methods around without any real change but GitHub is unfortunately not able to compare the files properly.

nopara73 commented 7 years ago

I made some comments in the code, but make some here, too.

Removed check File.Exists before calling File.Delete as it's not ne… … …cessary

Actually I did that intentionally. It doesn't affect performance but improve readability. But in the end whatever, let's use simply File.Delete()

Moved properties to the top, followed by the constructor and the static methods.

Be careful with Resharper automated stuff. I once spent half a day figuring out what the hell is happening and found Resharper fucked up a linq query (foreach -> aggregate) and that what caused the bug. I removed Resharper and was coding for 1 year without it. Just added it back recently.

Simplified the retrieval of wallets used for testing

I commented it inline.

removed unnecessary object creation in WalletFileserializer

Nice catch.

refactored the Safe.Load() method

I couldn't catch any difference. What do you mean?

I am picky with Safe related stuff, because:
The Safe is a pretty well-written class and the concept is fairly clear.
The Safe is already used.
The Safe's code is not messy.
The Safe has no bugs.
The Safe is the most important, security-wise, if something is fucked up here that is catastrophic.

I won't be picky with not Safe-related stuff, because you are more experienced developer and it's full of inefficiencies, vague concepts and messy code. So as long as the tests are passing everything is fine.
Why is the SPV stuff is messy, you may ask?

bokobza commented 7 years ago

Moved properties to the top, followed by the constructor and the static methods.

I did this manually, I haven't moved too many things, just a couple of properties, the constructor and the static methods.

refactored the Safe.Load() method

I just removed the creation of variables that were only used once, and move the assignment of some variables closer to where they're actually used. It just makes the code more readable sometimes :-) Otherwise I didn't change any functionality.

Why is the SPV stuff is messy, you may ask?

Is it? Haven't seen that yet.

you are more experienced developer

Not sure about that, but I know that programming on our own is quite hard so if you're happy for me to challenge you from time to time so that your library will be even better, then we're in a good position to make something even greater.

nopara73 commented 7 years ago

My new commits broke the existing saved datastructure. (AddressManager and HeaderChain files need not to be deleted, but the rest.)

bokobza commented 7 years ago

closing this for now, will come back to it later.