chrisdutz commented on code in PR #2674:
URL: https://github.com/apache/plc4x/pull/2674#discussion_r3724274096
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -249,16 +249,13 @@ private CompletableFuture<Void> probeAttributes() {
CipRRData eipWrapper = new CipRRData(sessionHandle,
CIPStatus.Success.getValue(),
DEFAULT_SENDER_CONTEXT, 0L, EMPTY_INTERFACE_HANDLE, 0, typeIds);
- return sendRequest(eipWrapper).thenAccept(response -> {
- if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue()) {
- return;
- }
- UnConnectedDataItem dataItem = (UnConnectedDataItem)
rr.getTypeIds().get(1);
- if (!(dataItem.getService() instanceof GetAttributeAllResponse
gar)) {
- return;
+ return sendRequest(eipWrapper).thenCompose(response -> {
+ if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue() ||
+ !(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di &&
di.getService() instanceof GetAttributeAllResponse gar)) {
+ return CompletableFuture.completedFuture(null);
}
if (gar.getStatus() == CIPStatus.ServiceNotSupported.getValue()) {
- return;
+ return checkAttributesSingle();
Review Comment:
Also leaving a few comments here to what's happeing would be great. I know I
didn't set a good example but I'm trying my best to leave more comments for my
fellow maintainers if I think something's tricky to understand. I guess the
problematic you're trying to solve would qualify for such a comment.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -249,16 +249,13 @@ private CompletableFuture<Void> probeAttributes() {
CipRRData eipWrapper = new CipRRData(sessionHandle,
CIPStatus.Success.getValue(),
DEFAULT_SENDER_CONTEXT, 0L, EMPTY_INTERFACE_HANDLE, 0, typeIds);
- return sendRequest(eipWrapper).thenAccept(response -> {
- if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue()) {
- return;
- }
- UnConnectedDataItem dataItem = (UnConnectedDataItem)
rr.getTypeIds().get(1);
- if (!(dataItem.getService() instanceof GetAttributeAllResponse
gar)) {
- return;
+ return sendRequest(eipWrapper).thenCompose(response -> {
+ if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue() ||
+ !(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di &&
di.getService() instanceof GetAttributeAllResponse gar)) {
+ return CompletableFuture.completedFuture(null);
}
if (gar.getStatus() == CIPStatus.ServiceNotSupported.getValue()) {
- return;
+ return checkAttributesSingle();
Review Comment:
Admittedly I find this particular part of code quite hard to read ... Could
you please simplify this a bit?
Admittedly I'm a big fan of the "di.getService() instanceof
GetAttributeAllResponse gar" notation saving myself the explicit cast, but I am
super unhappy with the decision of the Java group in a negated form to make the
variable available outside the if statement (Which you are using) ... it's just
challenging from a maintenance perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
