Add Comparable to Address remove from PrefixedChecksummedBytes #2018
Conversation
| int result = this.params.getId().compareTo(o.params.getId()); | ||
| if (result != 0) return result; | ||
|
|
||
| // Then compare Legacy vs Segwit |
There was a problem hiding this comment.
Couldn't the Legacy/Segwit comparison go to Address.compareTo()?
There was a problem hiding this comment.
Yeah, we could put that there and then call an abstract method with a different signature that the two subclasses would implement. I'm going to take a break from work for the next week, so I'll look at doing this when I return.
There was a problem hiding this comment.
How about this: 62ab82f ?
There was a problem hiding this comment.
Yes, that's the general idea. But is there a reason for the additonal method? Couldn't it all be contained in three compareTo() impls? The leaves would call super.compareTo() for the comparison on Address level.
There was a problem hiding this comment.
Yes, that's the general idea. But is there a reason for the additonal method? Couldn't it all be contained in three
compareTo()impls? The leaves would callsuper.compareTo()for the comparison onAddresslevel.
I thought about doing it that way (1), but decided against it because (a) it seemed wrong to have an implementation of compareTo for Address that (as a standalone method, that would be inherited by subclasses) was incorrect. It also leaves no unimplemented abstract method that subclasses might implement.
Instead I implemented (2) a compareTo method that along with the abstract compareAddressTo method forces subclasses to implement comparison between two objects of the subclass type.
A third way of doing it (3) would be to have an abstract compareTo in Address and provide a protected method that the subclass implementation of compareTo can call that will provide the ordering by netParams and subclass.
(3) might be a better way to go. I want to get this fixed, so I'm happy with any of the methods, but after implementing (2), I would say that (3) is my first choice, (2) my second choice, and (1) my last choice.
So let me know what you want me to do. I can implement (3) or (1) or leave it as it is.
There was a problem hiding this comment.
After your explanation I also think that (1) is clearly inferiour. As for picking (2) or (3), I think I have no preference so I'll leave it up to you.
There was a problem hiding this comment.
Thanks @schildbach , (3) is implemented (along with some JavaDoc cleanup), squashed, and is passing tests.
core/src/main/java/org/bitcoinj/core/PrefixedChecksummedBytes.java
Outdated
Show resolved
Hide resolved
|
@schildbach Can you review this one? If it looks good, I'll squash it, add a few more comments, and change it from "DRAFT" to "Ready to Merge". |
Require address subclasses to implement compareToand provide the compareAddressPartial method for comparing the first two fields.
msgilligan
force-pushed
the
msgilligan-fix-address-compareto
branch
from
a8ea460
to
7c6a37c
Compare
|
@schildbach This is ready for final review. |
msgilligan
changed the title
|
I removed a few unused imports from this PR and merged. Thanks for sticking to this! |
bitcoinj/bitcoinj#2018 is merged in bitcoinj-0.16, so this workaround can be removed.
This is a draft implementation of my proposed solution discussed in PR #1854.