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

Refactor GossipRouter: extract GossipRouterBuilder #249

Merged
merged 11 commits into from
Jul 18, 2022

Conversation

Nashatyrev
Copy link
Collaborator

@Nashatyrev Nashatyrev commented Jul 18, 2022

This is follow up to #247

In the very initial version I've made an attempt to invent a new Kotlin specific pattern which combines both builder and its resulting class entities. It should in theory make the code smaller and supporting efforts cheaper.

But it finally failed to be both a builder pattern and any kind of sustainable code. With GossipRouter becoming more complex class that approach is turning into error-prone spaghetti code.

This PR extracts GossipRouterBuilder with making GossipRouter properties final (where appropriate), which looks more like a traditional pattern for Teku code and should be less error-prone and more sustainable

Also:

  • Move property declarations right after primary constructor (which may also declare properties) in changed classes
  • Refactor AbstractRouter.addPeerWithDebugHandler() workaround
  • Remove pubsub interface var properties
  • Adjust tests

@Nashatyrev Nashatyrev changed the title Refactor GossipRouter: add a builder Refactor GossipRouter: extract GossipRouterBuilder Jul 18, 2022
@Nashatyrev Nashatyrev marked this pull request as ready for review July 18, 2022 13:31
Copy link
Collaborator

@tbenr tbenr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Nashatyrev Nashatyrev merged commit c9f3dfc into libp2p:develop Jul 18, 2022
@Nashatyrev Nashatyrev deleted the refactor/gossip-router-builder branch July 19, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants