Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Refactored Peer class #77

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Refactored Peer class #77

merged 1 commit into from
Jun 9, 2020

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Jun 5, 2020

This PR refactors the Peer class from the RLPx package.

Following changes:

  • Separate function for socket date processing (_onSocketData), extract from constructor code
  • Split up onSocketData functionality into separate _handleAuth, _handleAck functions
  • Split up parsePacketContent to separate handleHeader and handleBody functions
  • Logical resort of the different functions
  • Split up _handleMessage into _handleHello, handleDisconnect, handlePing and handlePong
  • Added basic function commentary
  • Additional debug output for errors catched to not let programmatic errors slip through unnoticed

Sorry if this is a bit hard to review, but this class needed some major revamp to have some fresh basis here. Tests pass and I also ran the example code on this for quite some time.

I would expect that we will find pre-existing bugs and suboptimalities on having a second look on the refactored code base. Before there was just too much "spaghetti" 馃槢 , especially along this central data processing code (now onSocketData).

@coveralls
Copy link

coveralls commented Jun 5, 2020

Coverage Status

Coverage increased (+0.002%) to 87.331% when pulling d9b8abc on some-refactoring into d77a3f9 on master.

@evertonfraga
Copy link

I am not sure I can review this thoroughly today, as I need to gather context from the library and dive deep. Will schedule it for monday. Sounds good @holgerd77?

@holgerd77
Copy link
Member Author

@evertonfraga Sure, definitely, neither this here nor the forkhash PR are too pressing.

@holgerd77
Copy link
Member Author

If this could get a review today or tomorrow would be nice. Would like to slowly start to plan for a maintenance release on the client (for next week or so), and this (respectively a subsequent devp2p patch release) would be some part of it.

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

refactoring looks great.
more concise and easier to read with smaller functions.
helpful debug messages.

@holgerd77 holgerd77 merged commit 711062f into master Jun 9, 2020
@holgerd77 holgerd77 deleted the some-refactoring branch June 9, 2020 18:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants