GetOutPointPrivateSendRounds readability #2149

PastaPastaPasta · 2018-06-26T14:35:51Z

Makes it easier to read GetOutPointPrivateSendRounds as it will now not have a magic 0 as the second arg when being called externally. Since the second arg is only used for the recursive logic, this should work well.

Since none of this should affect functionality, this should be able to be merged whenever.

…on by not having the second argument be needed, as the second arg should only not be 0 when it is recursively calling itself

thephez · 2018-06-26T14:44:44Z

.gitignore

@@ -40,6 +40,7 @@ src/config/dash-config.h.in
 src/config/stamp-h1
 share/setup.nsi
 share/qt/Info.plist
+/.vs


Nit: I know this is trivial, but it is unrelated to the actual change. Generally would be better to keep the PR focused on one specific thing 🙂

yeah, I normally try and avoid that. Didn't think it'd make sense to make a full PR for that tho and couldn't find anyone being too upset about it

Also includes an addition to the .gitnore for us weirdos who use visual studio... Don't judge me.

Just an FYI, this is generally done at a global level, instead of per-repo. For this PR, I think this needs to be removed as our guidelines specifically state:

Patchsets should always be focused. For example, a pull request could add a feature, fix a bug, or refactor code; but not a mixture. Please also avoid super pull requests which attempt to do too much, are overly large, or overly complex as this makes review difficult.

The reason is that it muddies history of features/fixes and makes it harder to track specific issues or revert specific commits -- if we needed to revert this later, we'd also have to revert the .gitignore addition.

nmarley · 2018-06-26T15:11:47Z

src/wallet/wallet.h

-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+	// get the PrivateSend chain depth for a given input
+    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint) const;
+	int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;


Please align properly

nmarley · 2018-06-26T15:14:14Z

src/wallet/wallet.cpp

@@ -1422,8 +1422,12 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
    }
    return 0;
 }
+// Starts the recursive function which determines the rounds of a given input (How deep is the PrivateSend chain for a given input)


Spacing needs to be retained, and not sure why you changed the comment.

nmarley · 2018-06-26T15:14:32Z

nACK

Thanks for this, but honestly I'm not convinced there's a need for it. Overloading this function to add a default nRounds = 0 doesn't really give us much and this just adds extra code without much (if any) benefit. I'd like to stick with cleaner and less code if possible, not more.

This reverts commit 129b524.

PastaPastaPasta · 2018-06-26T15:27:13Z

Changed back comment, after rereading the old one is fine. fixed alignment, darn tabbing. removed the .gitnore change, thanks for explaining why. That reasoning makes sense

I personally disagree with it not providing any value. When reading the code, it makes no sense for that zero to be there, and requires the reader to look into the function and figure out what that zero actually means, whereas it is clear why the output is there and what it does. Also, in regards to writing, it becomes quite possible to forget that the zero needs to be there, again wasting time. I personally messed that up in a prior PR because it doesn't intuitively make sense for the zero to be present.

nmarley · 2018-06-26T15:41:27Z

requires the reader to look into the function and figure out what that zero actually means

That should be the case regardless for any contributors to the codebase.

it becomes quite possible to forget that the zero needs to be there, again wasting time

That's why function signatures exist and are checked at compile time.

PastaPastaPasta · 2018-06-26T15:47:31Z

Alright, well. I've made my case, I think this makes it easier to read without adding any real negatives. You guys can decide how you wish :)

nmarley · 2018-06-26T15:49:40Z

I personally disagree with it not providing any value. When reading the code, it makes no sense for that zero to be there, and requires the reader to look into the function and figure out what that zero actually means, whereas it is clear why the output is there and what it does. Also, in regards to writing, it becomes quite possible to forget that the zero needs to be there, again wasting time. I personally messed that up in a prior PR because it doesn't intuitively make sense for the zero to be present.

This is not a valid argument. Any C++ developer will understand the functions they're calling before using them. For complicated code, refactoring makes sense, but this is just setting a default.

Comments can serve to help understand why a block of code is written the way it is, or what the intended use of a variable is, but "forgetting" a parameter alone is a terrible excuse for a function overload. Most IDEs will detect this and certainly the compiler does at compile time.

There's really not any excuse for not understanding the code you're contributing to.

PastaPastaPasta · 2018-06-26T16:36:56Z

This is not a valid argument. Any C++ developer will understand the functions they're calling before using them. For complicated code, refactoring makes sense, but this is just setting a default.

Sure, and this is nothing major. But I don't understand how you don't see the slight improvement this makes to readability. Seeing the output there makes complete sense, seeing a zero there doesn't and forces you to spend time figuring out what that zero actually does. Same with usage, I expect to pass an output. I don't expect to pass some int, wasting time forcing me to see what exactly that int does. Sure a good dev will know what the method does and how exactly to manipulate it, but people forget and some devs aren't as good as you all 😆.

The reason I think this is a valid change is that the second parameter (i think) will never be used externally. It will always be zero, only in the recursive loop will it change. It seems a lot cleaner to me to overload it.

Most IDEs will detect this and certainly the compiler does at compile time.

Sure IDEs will catch it or the compiler will. But again, that wastes time. I just don't see the point for it.

There's really not any excuse for not understanding the code you're contributing to.

Of course you should understand it as much as you can. But I think it is also contributors' job to make sure that it is easy to understand for people who aren't as familiar, which I think this change does without any negative effects

UdjinM6 · 2018-06-26T16:50:21Z

@paulied I see what you are trying to achieve but imo it's a weird way (no offence :) ).

Given that I'd rather go with smth like

     int  CountInputsWithAmount(CAmount nInputAmount);
 
     // get the PrivateSend chain depth for a given input
-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds = 0) const;
     // respect current settings
    int GetOutpointPrivateSendRounds(const COutPoint& outpoint) const;

In fact, if you really want to fix readability of Get*OutpointPrivateSendRounds() usage, I would go even further with smth like

     int  CountInputsWithAmount(CAmount nInputAmount);
 
     // get the PrivateSend chain depth for a given input
-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds = 0) const;
     // respect current settings
-    int GetOutpointPrivateSendRounds(const COutPoint& outpoint) const;
+    int GetCappedOutpointPrivateSendRounds(const COutPoint& outpoint) const;
 
     bool IsDenominated(const COutPoint& outpoint) const;

and then would refactor all the code using these functions, so that it was 100% clear what number we are using exactly in each particular case. Or smth like that, more or less :)

PastaPastaPasta · 2018-06-26T17:43:24Z

Refactored based on @UdjinM6's response. I forgot about setting a default parameter, never used it much before. Thanks for that. Definitely better then my approach
I think the suggestion to make the other one 'Capped' is quite good and describes what the method actually does better

UdjinM6

Looks better imo, few more things to fix though, see below.

UdjinM6 · 2018-06-26T17:39:55Z

src/wallet/wallet.cpp

@@ -1423,7 +1423,12 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
    return 0;
 }

-// Recursively determine the rounds of a given input (How deep is the PrivateSend chain for a given input)
+// Recursively determine the rounds of a given input (How deep is the PrivateSend chain for a given input)	
+int CWallet::GetRealOutpointPrivateSendRounds(const COutPoint& outpoint) const


This should be removed, no need to overload GetRealOutpointPrivateSendRounds

UdjinM6 · 2018-06-26T17:42:01Z

src/wallet/wallet.h

@@ -819,10 +819,10 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
    bool HasCollateralInputs(bool fOnlyConfirmed = true) const;
    int  CountInputsWithAmount(CAmount nInputAmount);

-    // get the PrivateSend chain depth for a given input
-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+	// get the PrivateSend chain depth for a given input


Keep it properly aligned here pls

nmarley · 2018-06-26T17:56:20Z

forces you to spend time figuring out what that zero actually does
wasting time forcing me to see what exactly that int does

I don't agree that it's a waste of time. It makes sense to have a holistic, not partial, understanding of the codebase. There might be a good reason to use that one random parameter which you're sure in this case will never be used "externally". (I'm honestly not sure what you mean by "externally" here, unless you mean referenced outside the class itself.)

Udjin's suggestion of using a default param is of course cleaner (but I would have opted to just add that to the existing method rather than add a new method). But I still don't see any readability issues with the original, and I think it's a reasonable expectation that contributors either be experienced with C++ development or quickly learn those skills.

This reverts commit c2cc2ae.

PastaPastaPasta · 2018-06-26T18:33:13Z

@UdjinM6, I think that is good now, sorry for the mess of commits.

nmarley · 2018-06-26T18:40:33Z

src/wallet/wallet.cpp

@@ -1423,7 +1423,7 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
    return 0;
 }

-// Recursively determine the rounds of a given input (How deep is the PrivateSend chain for a given input)
+// Recursively determine the rounds of a given input (How deep is the PrivateSend chain for a given input)	


Can you remove the trailing spaces here?

nmarley · 2018-06-26T18:41:16Z

Mostly looks good at 635de9b -- I just see some trailing spaces in one line that should be removed.

schinzelh · 2018-06-26T18:42:32Z

@paulied no worries, that's what PR branches are for :)
BTW: I'd suggest to look into squashing commits with git, that way you can keep your PRs "clean"

And last but not least: Thanks for contributing, much appreciated.

…thub.com/PaulieD/dash into GetOutpointPrivateSendRounds-readability

PastaPastaPasta · 2018-06-26T19:02:56Z

Extra spacing removed.

Thanks @schinzelh, thanks for the tip. Glad to be able to help out, hopefully xd

UdjinM6

Looks good, thanks! 👍

utACK for 12.4

PS. if that merge commit was meant to be a squash one then it's not how you do it ;) but nvm, we'll just squash this all on PR merge instead.

PastaPastaPasta · 2018-06-26T19:17:52Z

Thanks @UdjinM6 :)

* .gitnore visual studio bs * Improves the readability of the `GetOutpointPrivateSendRounds` function by not having the second argument be needed, as the second arg should only not be 0 when it is recursively calling itself * Revert ".gitnore visual studio bs" This reverts commit 129b524. * changed back comment and fixed allignment * refactor based on Udjin's suggestions. * refactor based on Udjin's suggestions. * fix alignment * Revert "fix alignment" This reverts commit c2cc2ae. * actually fix alignment * actually fix alignment

Paul added 2 commits June 26, 2018 09:17

.gitnore visual studio bs

129b524

Improves the readability of the GetOutpointPrivateSendRounds functi…

5f4e3b3

…on by not having the second argument be needed, as the second arg should only not be 0 when it is recursively calling itself

thephez reviewed Jun 26, 2018

View reviewed changes

nmarley reviewed Jun 26, 2018

View reviewed changes

Paul added 2 commits June 26, 2018 10:16

Revert ".gitnore visual studio bs"

8e8a75c

This reverts commit 129b524.

changed back comment and fixed allignment

d71435e

refactor based on Udjin's suggestions.

72944f1

UdjinM6 requested changes Jun 26, 2018

View reviewed changes

UdjinM6 added this to the 12.4 milestone Jun 26, 2018

Paul added 5 commits June 26, 2018 12:57

refactor based on Udjin's suggestions.

be582ad

fix probs

28174d1

fix alignment

c2cc2ae

Revert "fix alignment"

89d771b

This reverts commit c2cc2ae.

actually fix alignment

635de9b

nmarley reviewed Jun 26, 2018

View reviewed changes

Paul added 2 commits June 26, 2018 14:00

actually fix alignment

6149a33

Merge branch 'GetOutpointPrivateSendRounds-readability' of https://gi…

9fa4445

…thub.com/PaulieD/dash into GetOutpointPrivateSendRounds-readability

UdjinM6 approved these changes Jun 26, 2018

View reviewed changes

UdjinM6 merged commit 448e92f into dashpay:develop Jul 12, 2018

UdjinM6 mentioned this pull request Jul 12, 2018

Fixes inaccurate round count in CoinControlDialog (Issue #2068) #2137

Merged

PastaPastaPasta deleted the GetOutpointPrivateSendRounds-readability branch September 15, 2018 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetOutPointPrivateSendRounds readability #2149

GetOutPointPrivateSendRounds readability #2149

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

thephez Jun 26, 2018

PastaPastaPasta Jun 26, 2018

nmarley Jun 26, 2018

nmarley Jun 26, 2018

nmarley Jun 26, 2018

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

UdjinM6 commented Jun 26, 2018 •

edited

Loading

PastaPastaPasta commented Jun 26, 2018

UdjinM6 left a comment

UdjinM6 Jun 26, 2018

UdjinM6 Jun 26, 2018 •

edited

Loading

nmarley commented Jun 26, 2018 •

edited

Loading

PastaPastaPasta commented Jun 26, 2018

nmarley Jun 26, 2018

nmarley commented Jun 26, 2018

schinzelh commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018

UdjinM6 left a comment

PastaPastaPasta commented Jun 26, 2018

GetOutPointPrivateSendRounds readability #2149

GetOutPointPrivateSendRounds readability #2149

Conversation

PastaPastaPasta commented Jun 26, 2018 • edited Loading

thephez Jun 26, 2018

Choose a reason for hiding this comment

PastaPastaPasta Jun 26, 2018

Choose a reason for hiding this comment

nmarley Jun 26, 2018

Choose a reason for hiding this comment

nmarley Jun 26, 2018

Choose a reason for hiding this comment

nmarley Jun 26, 2018

Choose a reason for hiding this comment

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018 • edited Loading

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018

nmarley commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018 • edited Loading

UdjinM6 commented Jun 26, 2018 • edited Loading

PastaPastaPasta commented Jun 26, 2018

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 Jun 26, 2018

Choose a reason for hiding this comment

UdjinM6 Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

nmarley commented Jun 26, 2018 • edited Loading

PastaPastaPasta commented Jun 26, 2018

nmarley Jun 26, 2018

Choose a reason for hiding this comment

nmarley commented Jun 26, 2018

schinzelh commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Jun 26, 2018

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

PastaPastaPasta commented Jun 26, 2018 •

edited

Loading

UdjinM6 commented Jun 26, 2018 •

edited

Loading

UdjinM6 Jun 26, 2018 •

edited

Loading

nmarley commented Jun 26, 2018 •

edited

Loading