Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can data finished #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Can data finished #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e878802
CAN_Data added
Jun 9, 2022
da46f49
CAN_CHECKED
Jun 9, 2022
86a9c84
Finish CAN_DATA
Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .idea/gradle.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .idea/misc.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions app/src/main/java/com/example/bottomnav/CAN_Data.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.example.bottomnav;

class CAN_Data {
int id;
int[] data;

public CAN_Data(int id, int[] data, int len) {
this.id = id;
this.data = data;
}

public static CAN_Data decode(String raw) {
int id = -1;
int len;

int index = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (raw.charAt(0) == 't' || raw.charAt(0) == 'r') {
id = Integer.parseInt(raw.substring(1, 4), 16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index = 4;
}
else if (raw.charAt(0) == 'T' || raw.charAt(0) == 'R') {
id = Integer.parseInt(raw.substring(1, 9), 16);
index = 9;
}
else {
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nulls can lead debugging nightmares, because there's nothing in the method signature that indicates that it can be null, so more often then not one might just assume that it'll be non-null.

Where you have a code path that can fail, I recommend encoding the possibility of failure in the type system, such as through an Optional type. I believe this is is more common in functional languages, but is more supported elsewhere now (including being part of the Java language)

}

len = Integer.parseInt(raw.substring(index, index + 1), 16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[] 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

}
return new CAN_Data(id, data, len);
}

public int getId(){
return id;
}

public int[] getData() {
return data;
}



Comment on lines +45 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

excessive newlines can be deleted

}
24 changes: 24 additions & 0 deletions app/src/test/java/com/example/bottomnav/DataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package com.example.bottomnav;

import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class DataTest {
@Test
public void randomData() {
String data = "t056111";
CAN_Data newData = CAN_Data.decode(data);
System.out.println(newData.getData());
assertEquals( 0x11 , newData.getData()[0]);
assertEquals( 86 , newData.getId());
}
@Test
public void BIGData() {
String data = "t0568111111111111111A";
CAN_Data newData = CAN_Data.decode(data);
System.out.println(newData.getData());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests shouldn't have printf, unit tests should be fully automated

assertEquals( 0x11, newData.getData()[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend checking all the payload data.
And all the parsed data - including ID.

assertEquals( 8, newData.getData().length);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.