chrisdutz commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500057457
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean isByte() {
+ return (value != null) && (value <= Byte.MAX_VALUE) && (value >=
Byte.MIN_VALUE);
+ }
+
+ @Override
+ @JsonIgnore
+ public byte getByte() {
+ return value.byteValue();
+ }
+
Review comment:
We now have a "byte getByte()" and a "short getBYTE()" method, I think
it should only be the one or the other ... I would like to opt for the "byte
getByte()". But right now I'm unsure which implications that would have.
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
Review comment:
In general a bit-string (at least in my perception of it, represents an
array of booleans. So if for example I read one byte, from the point of the API
it should be a list of 8 boolean values ... I think we need to allow this
somehow. Right now the only way would be to get the "byte" value and to pick
that appart.
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -181,6 +205,90 @@ public short getShort() {
return value;
}
+ @Override
+ @JsonIgnore
+ public boolean isInteger() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public int getInteger() {
+ return value.intValue();
+ }
+
Review comment:
Instead of adding these for every type, wouldn't it make sense to have
something like a "PlcIECBitStringValue", "PlcIECIntegerValue", ...?
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -181,6 +205,90 @@ public short getShort() {
return value;
}
+ @Override
+ @JsonIgnore
+ public boolean isInteger() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public int getInteger() {
+ return value.intValue();
+ }
+
Review comment:
But thinking of it ... perhaps making it generic and passing in the type
would help ...
##########
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcValues.java
##########
@@ -355,6 +356,28 @@ public static PlcValue of(Map<String, PlcValue> map) {
return new PlcStruct(map);
}
+ private static PlcValue constructorHelper(Constructor<?> constructor,
Object value) {
+ try {
+ return (PlcValue) constructor.newInstance(value);
+ } catch (InstantiationException | IllegalAccessException |
InvocationTargetException e) {
+ throw new PlcIncompatibleDatatypeException(value.getClass());
+ }
+ }
+
+ public static PlcValue of(Object[] values, Class clazz) {
+ //Encode values to the type defined in clazz
+ try {
+ Constructor<?> constructor =
clazz.getDeclaredConstructor(values[0].getClass());
+ if(values.length == 1) {
+ return ((PlcValue) constructor.newInstance(values[0]));
+ } else {
+ return PlcValues.of(Arrays.stream(values).map(value ->
(constructorHelper(constructor, value))).collect(Collectors.toList()));
+ }
Review comment:
One thing I've learnt recently, is that exceptions are extremely
expensive, as creating the stack trace is expensive. is this code that is
likely to produce any of "InstantiationException | IllegalAccessException |
InvocationTargetException | NoSuchMethodException" in normal execution? If yes,
we should probably change this and other parts where we use similar patterns to
check first instead of silently catching exceptions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
