Can data finished #8
Conversation
There was a problem hiding this comment.
Overall, this looks like it will work fine, but I think it could use some robustness (see inline comments). Especially since data is coming in from wireless, it may be possible to get malformed or corrupted data, and your parser should be tolerant of this (ie not crash), even if all it does is throw up its hands and return an error or empty type.
| @@ -7,6 +7,7 @@ | |||
| <option name="testRunner" value="GRADLE" /> | |||
| <option name="distributionType" value="DEFAULT_WRAPPED" /> | |||
| <option name="externalProjectPath" value="$PROJECT_DIR$" /> | |||
| <option name="gradleJvm" value="16" /> | |||
There was a problem hiding this comment.
Can you back out this unrelated change?
| @@ -15,7 +15,7 @@ | |||
| </map> | |||
| </option> | |||
| </component> | |||
| <component name="ProjectRootManager" version="2" languageLevel="JDK_11" default="true" project-jdk-name="Android Studio default JDK" project-jdk-type="JavaSDK"> | |||
| <component name="ProjectRootManager" version="2" languageLevel="JDK_1_8" default="true" project-jdk-name="Android Studio default JDK" project-jdk-type="JavaSDK"> | |||
| assertEquals( 0x11, newData.getData()[0]); | ||
| assertEquals( 8, newData.getData().length); | ||
| } | ||
| } |
There was a problem hiding this comment.
To be thorough, can you also add tests for the extended case?
In general, I'd advocate unit testing all code paths that are easily testable. I believe that in software engineering practice it isn't uncommon to have as much (or even more) testing code as actual application code.
Also, consider testing malformed inputs. Because of the nature of a wireless connection, it may be possible to get partial (possibly even corrupted) data, so your parser needs to be tolerant. It doesn't need to decode the data successfully (which would be impossible if it were corrupted), but it does need to not crash and not give misleading results.
| String data = "t0568111111111111111A"; | ||
| CAN_Data newData = CAN_Data.decode(data); | ||
| System.out.println(newData.getData()); | ||
| assertEquals( 0x11, newData.getData()[0]); |
There was a problem hiding this comment.
I'd recommend checking all the payload data.
And all the parsed data - including ID.
| public void BIGData() { | ||
| String data = "t0568111111111111111A"; | ||
| CAN_Data newData = CAN_Data.decode(data); | ||
| System.out.println(newData.getData()); |
There was a problem hiding this comment.
Unit tests shouldn't have printf, unit tests should be fully automated
| int id = -1; | ||
| int len; | ||
|
|
||
| int index = -1; |
There was a problem hiding this comment.
Can you leave this uninitialized? It doesn't really need to be -1?
Similarly with id.
Hopefully if the IDE is good, it can also warn you if you end up using an uninitialized variable, whereas if you give it a default it won't be able to warn you.
| return null; | ||
| } | ||
|
|
||
| len = Integer.parseInt(raw.substring(index, index + 1), 16); |
There was a problem hiding this comment.
Consider adding a validation step to make sure:
- that the input is long enough such that it has a length field
- that the length is 8 or less
- that the input is long enough for the entire payload
and if it fails validation, return Optional.empty()
Also, here (and every other instance of parseInt) consider having error handling, specifically catching the NumberFormatException that parseInt can throw and returning Optional.empty(). You might as well wrap the entire method in a try-catch or something, since you use a lot of parseInt.
|
|
||
| int index = -1; | ||
| if (raw.charAt(0) == 't' || raw.charAt(0) == 'r') { | ||
| id = Integer.parseInt(raw.substring(1, 4), 16); |
There was a problem hiding this comment.
Consider adding validation here, to check that the input contains enough characters to decode the ID, and optionally the resulting decoded ID is within the 11-bit standard ID format defined by CAN.
| int[] data = new int[len]; | ||
|
|
||
| for(int x = 0;x<len;x+=1) { | ||
| data[x] = Integer.parseInt(raw.substring(2*x +index+1, 2*x+2+index+1), 16); |
There was a problem hiding this comment.
| data[x] = Integer.parseInt(raw.substring(2*x +index+1, 2*x+2+index+1), 16); | |
| data[x] = Integer.parseInt(raw.substring(ndex+1 + 2*x, index+1 + 2*x + 2), 16); |
I think this should clarify the indexing a bit more?
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
excessive newlines can be deleted
Checking CAN DATA for merge to main