andvasp commented on code in PR #2674:
URL: https://github.com/apache/plc4x/pull/2674#discussion_r3731278094
##########
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.
Here I see 3 options, where I prefer the sequential other. What do you think?
Option 1:
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);
}
Option 2:
if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue()) {
return CompletableFuture.completedFuture(null);
}
if (!(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di) ||
!(di.getService() instanceof GetAttributeAllResponse gar)) {
return CompletableFuture.completedFuture(null);
}
Option 3:
if (!(response instanceof CipRRData rr) || rr.getStatus() !=
CIPStatus.Success.getValue()) {
return CompletableFuture.completedFuture(null);
}
UnConnectedDataItem dataItem = (UnConnectedDataItem) rr.getTypeIds().get(1);
if (!(dataItem.getService() instanceof GetAttributeAllResponse gar)) {
return CompletableFuture.completedFuture(null);
}
--
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]
