plc4x-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [plc4x] chrisdutz commented on a change in pull request #192: Refactor Field Handler Classes
Date Tue, 06 Oct 2020 07:33:43 GMT

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:
users@infra.apache.org



Mime
View raw message