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

WIP: ForwardingService refactoring for JDK 11 with record-like Config class. #3378

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

msgilligan
Copy link
Member

@msgilligan msgilligan commented Apr 26, 2024

This is WIP for an improved JDK 11 ForwardingService

Overall this results in (IMHO) a very clean example:

  1. The main() method is short but clearly shows overall flow of the example
  2. The main() method includes all (main thread) stdout. (The other stdout is really log functionality, but since this example doesn't use a logging framework were using stdout)
  3. The Config object is fully-immutable and is 100% compatible with the record class
  4. The ForwardingService object has no mutable members and is self-contained
  5. There are zero mutable/mutated variables (they're either final or effectively final)

Copy link
Member Author

@msgilligan msgilligan left a comment

Choose a reason for hiding this comment

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

I can't review my own PR, but I found some things that need changing.

core/src/main/java/org/bitcoinj/base/BitcoinNetwork.java Outdated Show resolved Hide resolved
integration-test/build.gradle Outdated Show resolved Hide resolved
compileJava.options.encoding = 'UTF-8'
compileTestJava.options.encoding = 'UTF-8'
javadoc.options.encoding = 'UTF-8'

compileJava {
options.compilerArgs << '-Xlint:deprecation'
options.compilerArgs.addAll(['--release', '17'])
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here.

settings.gradle Outdated Show resolved Hide resolved
@msgilligan msgilligan force-pushed the msgilligan/ForwardingService-refactor-record-like branch 2 times, most recently from ff4efcc to 547c8c8 Compare April 27, 2024 20:24
* examples, integration-test: Gradle use JDK 17+
* ForwardingService: Use Java 17 to simplify

Simplification:

* Move more code back to main method
* Use record class to collect config params
* static parse(args) method to build config
* Construct ForwardingService with Wallet and Config objects
* Use effectively final objects in the main try-with-resources
* Every variable -- except two locals in parse() -- is either final
  or effectively final.
* Keep important stuff towards the top of the file, details at
  the bottom.

The overall objective of this refactoring was to make the overall
flow clearly visible in the main method of the app while preserving
ForwardingService as a "reusable" and `Closable` object. I realize
that things could be further simplified by moving more code into
main, but I want this example to encourage creating modular "services"
with bitcoinj.
This also allows moving two constants inside the record class.
Also add BitcoinNetwork.supports(Address)
Rewrite forward() to be a more generic send() method (which is essentially
just a wrapper to wallet.sendTransaction() that logs various completion states.

Note that we moved the whenComplete blog that logs "relayed" status so users
can use this code in more cases. This is because a common question that
comes up is "why was my transaction not seen?". So making this logging
code more reusable should be helpful.
@msgilligan msgilligan force-pushed the msgilligan/ForwardingService-refactor-record-like branch from 547c8c8 to 48e09ac Compare May 14, 2024 18:32
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