Removing dead code from the consensus package #554
Conversation
CjHare
changed the title
|
|
||
| public class SignerRateLimitValidationRuleTest { | ||
|
|
||
| // Implicitly conducted by NodeCanProduceNextBlockTest. |
There was a problem hiding this comment.
Instead of deleting the test can we add a small test for this validation
There was a problem hiding this comment.
...but if you have a test in mind, then don't let me stop you ;)
...nsus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/CliqueBlockHashingTest.java
Outdated
Show resolved
Hide resolved
| @@ -55,11 +55,6 @@ public void discard(final Address address) { | |||
| proposals.remove(address); | |||
| } | |||
|
|
|||
| /** Discards all pending votes */ | |||
There was a problem hiding this comment.
Do we want to keep this here as a symmetry issue - i.e. even if not used, we should offer the functions assumed of a list style class.
There was a problem hiding this comment.
No.
On this point I could extoll the virtues of minimalism or the ideals of software craftmanship but ultimately it’s most basic form, it’s cost / benefit.
The intent of this class, like most others in the consensus package, are not to be packaged and published for general consumption as an API. (i.e. compare for example our approach to the libraries of Vertex, Spring or Apache commons).
The cost here is writing, testing (I know I’m assumptions there, which may, or may not hold true) and maintaining code.
What is the benefit? The code is not being used, it is not intended for public use as an API. In the future someone may have use it, but equally they may not.
When the benefit of something is close to zero, no matter the cost it’s a poor deal.
Does that perspective make sense?
| @@ -137,10 +135,6 @@ public void setRoundChangeCertificate(final RoundChangeCertificate roundChangeCe | |||
| this.roundChangeCertificate = roundChangeCertificate; | |||
| } | |||
|
|
|||
| public void setProposalPayload(final SignedData<ProposalPayload> proposalPayload) { | |||
There was a problem hiding this comment.
Given this is a builder, it should offer the ability to set all fields, even if not used.
There was a problem hiding this comment.
Will move & investigate on how it's used in the tests
| @@ -51,18 +49,6 @@ public MessageValidator createMessageValidator( | |||
| parentHeader); | |||
| } | |||
|
|
|||
| public RoundChangeMessageValidator createRoundChangeMessageValidator( | |||
There was a problem hiding this comment.
For completeness this should remain.
There was a problem hiding this comment.
Perhaps we should be using this in the RoundChangeManager instead of using the constructor.
There was a problem hiding this comment.
How do you mean? (I'm not seeing RoundChangeManager reference the MessageValidatorFactory)
There was a problem hiding this comment.
Discussion outcome: will rollback this delete
| @@ -227,20 +227,6 @@ public void lastestPreparedCertificateMatchesNewRoundProposalIsSuccessful() { | |||
| final SignedData<ProposalPayload> proposal = | |||
| proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock); | |||
|
|
|||
| final ConsensusRoundIdentifier preparedRound = | |||
There was a problem hiding this comment.
this needs to go back in, the preparedCertificate should be being used in the RoundChange
There was a problem hiding this comment.
Are you saying that the test 'should' be using the preparedCert but wasn't?
There was a problem hiding this comment.
...as the removed code, definitely was unreferenced.i.e. not being used
There was a problem hiding this comment.
After discussion: will rewrite the test to have both certs in the proposal
| @@ -1,42 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Given this is from ibftLegacy, I'd be tempted to keep this - Legacy was never completed, so will have dead code.
There was a problem hiding this comment.
What is IbftLegacy? Why keep code in the repo for functionality that was never completed?
…o removing_unused_code
…lliJ
PR description
Removing code identified by the 'unused declaration' analysis in IntelliJ in the consensus package (main, test & integration-test)