refactor: Modernize ChaincodeBase class #357
Conversation
Signed-off-by: Bhaskar Ram <bhaskar.ram@linux.com>
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
Several of your changes use language features not available in Java 8. Did you try to compile this code using the Gradle configuration, or to run unit tests?
fabric-chaincode-shim/src/main/java/org/hyperledger/fabric/shim/ChaincodeBase.java
Show resolved
Hide resolved
fabric-chaincode-shim/src/main/java/org/hyperledger/fabric/shim/ChaincodeBase.java
Outdated
Show resolved
Hide resolved
| Optional.ofNullable(this.getClass().getPackage()) | ||
| .ifPresentOrElse( | ||
| pkg -> Logger.getLogger(pkg.getName()).setLevel(chaincodeLogLevel), | ||
| () -> Logger.getLogger("").setLevel(chaincodeLogLevel) | ||
| ); |
There was a problem hiding this comment.
I don't see any real benefit to using an optional to perform an if/else, and I don't think this is available in Java 8
There was a problem hiding this comment.
Replaced the Optional usage with a simple null check.
Uses a standard if/else statement, which is more readable and familiar to most Java developers. Now in the latest commit It Is compatible with Java 8 and earlier versions.
| return switch (level.toUpperCase().trim()) { | ||
| case "CRITICAL", "ERROR" -> Level.SEVERE; | ||
| case "WARNING", "WARN" -> Level.WARNING; | ||
| case "INFO" -> Level.INFO; | ||
| case "NOTICE" -> Level.CONFIG; | ||
| case "DEBUG" -> Level.FINEST; | ||
| default -> Level.INFO; | ||
| }; |
There was a problem hiding this comment.
This looks reasonable but the project uses the Java 8 language level, which does not support switch expressions
There was a problem hiding this comment.
Updated in latest commit.
| Optional.ofNullable(System.getenv(CORE_PEER_ADDRESS)) | ||
| .ifPresent(address -> { | ||
| String[] hostArr = address.split(":"); | ||
| if (hostArr.length == 2) { | ||
| this.port = Integer.parseInt(hostArr[1].trim()); | ||
| this.host = hostArr[0].trim(); | ||
| } else { | ||
| LOGGER.severe(() -> String.format("peer address argument should be in host:port format, ignoring current %s", address)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I don't see any real benefit to using an optional only to perform a null check
There was a problem hiding this comment.
Replaced the Optional.ofNullable().ifPresent() with a simple null check using an if statement. Improved readability by using a more straightforward and familiar Java construct.
| LOGGER.info(() -> String.format(""" | ||
| <<<<<<<<<<<<<Environment options>>>>>>>>>>>> | ||
| CORE_CHAINCODE_ID_NAME: %s | ||
| CORE_PEER_ADDRESS: %s | ||
| CORE_PEER_TLS_ENABLED: %s | ||
| CORE_PEER_TLS_ROOTCERT_FILE: %s | ||
| CORE_TLS_CLIENT_KEY_PATH: %s | ||
| CORE_TLS_CLIENT_CERT_PATH: %s | ||
| CORE_TLS_CLIENT_KEY_FILE: %s | ||
| CORE_TLS_CLIENT_CERT_FILE: %s | ||
| CORE_PEER_LOCALMSPID: %s | ||
| CHAINCODE_SERVER_ADDRESS: %s | ||
| LOGLEVEL: %s | ||
| """, |
There was a problem hiding this comment.
Text blocks are not available in Java 8
There was a problem hiding this comment.
Latest Commit is now fully compatible with Java 8.
Signed-off-by: Bhaskar Ram <bhaskar.ram@linux.com>
|
|
@bestbeforetoday I've added recent changes for compatibility for Java 8, please review it. |
|
The build is failing since the code does not even compile. Looking at the current set of changes (after the use of language features not available in Java 8 are reverted), the vast majority of the changes are reformatting of the code. A couple of variables are renamed and, in a couple of cases, the logic flow of the code is slightly reordered. What issue is this change addressing? |
|
No response so closing. |
3 participants
refactor: Modernize ChaincodeBase class
Signed-off-by: Bhaskar Allam bhaskar.ram@linux.com