# Parsing CAN frames from a ByteArray

Here's my method used for parsing a ByteArray into multiple frames of CAN interface. It works now correctly but it's somewhat ugly in my opinion.

It's written in Kotlin with use of RxJava2

private fun parseCanFrameBytes(frame: ByteArray): Observable<CanFrame> {
return Observable.create { emitter ->
var index = 0
while (index < frame.size) {
val byte = frame[index]

if(byte xor IFM_FRAME_HEADER == BYTE_ZERO) {
try {
val ifmIndex = index
val length = frame[ifmIndex + 1].toInt()
val dlc1 = length - 3
val dlc2 = length - 5
val command = frame[ifmIndex + 2].toInt()
val data = frame.copyOfRange(ifmIndex + 3, ifmIndex + 2 + length)
val isChecksumValid = frame[ifmIndex + 2 + length].toInt() == frame.copyOfRange(ifmIndex, ifmIndex + 2 + length).calculate8XorChecksum()
if (!isChecksumValid) {
index++
continue
}

val canFrame = when (command) {
0 -> {
val pgn = data.copyOfRange(0, 2).toHexString()
CanFrame(
type = "DLC1",
pgn = pgn,
id = pgn,
value = data.copyOfRange(2, 2 + dlc1).toHexString(),
ts = timestamp(),
data = frame.copyOfRange(ifmIndex, ifmIndex + 4 + length).toHexString()
)
}
2 -> {
val pgn = data.copyOfRange(0, 4).toHexString()
CanFrame(
type = "DLC2",
pgn = pgn,
id = pgn,
value = data.copyOfRange(4, 4 + dlc2).toHexString(),
ts = timestamp(),
data = frame.copyOfRange(ifmIndex, ifmIndex + 4 + length).toHexString()
)
}
else -> null
} ?: continue

emitter.onNext(canFrame)
index = ifmIndex + length + 4
} catch (ex: IllegalArgumentException) {
index++
}
} else {
index++
}
}
}
}

• Can you add some more context, what exactly is CanFrame ? – Simon Forsberg Jun 21 '18 at 14:08
• I don't think that's important at all. What I want to achieve is more concise and better looking code. It's pretty niche thing but if you want you can read about it here en.wikipedia.org/wiki/CAN_bus – Ernest Zamelczyk Jun 21 '18 at 14:13
• I'd recommend you to read Simon's guide to a good question – Simon Forsberg Jun 21 '18 at 16:20

### Nesting Branches

You were worried your code might be a bit ugly. I consider deeply nested branches ugly aka a penalty in readability when they can easily be avoided. For example, your function indents the entire while-loop body to perform this little branch at the end:

if (byte xor IFM_FRAME_HEADER == BYTE_ZERO) {
// branch body ..
} else {
index++
}


You could invert the branch condition, use an early continue and avoid nesting branches.

if (byte xor IFM_FRAME_HEADER != BYTE_ZERO) {
index++
continue
}
// other code in while-loop no longer needs indentation ..


### Shifting the input

You have 3 occasions where you shift the input array to the next index index++. 2 of these have conditions that guard when to shift, the 3th is an exception handler for type IllegalArgumentException. Consider refactoring the code to have only a single connection point in the code where you perform the shift index++. This increases maintainability of the code. You can do this be either (1) trying to avoid the exception and using a condition instead, and use a single flag shouldShift marked true on any of the conditions or (2) throw exceptions for all conditions that require shifting and have a single exception handler performing the shift.

### Flow Integrity

The callback invocation emitter.onNext(canFrame) sits within the try-catch block. This means that if the callback would throw an IllegalArgumentException you'd be shifting the input index++ while you shouldn't have. Ensure callback invocation does not trigger such shifting.

Probably very little beautification. I would be curious to see someone beautify it and prove me wrong. Initially I thought you could use Kotlin streams on the frame ByteArray. This way you could have done something equivalent to frame.filterIndexed(..).map(it->CanFrame(..)) etc. However then I noticed you were doing random access on the ByteArray: ex: ifmIndex + length + 4 , frame.copyOfRange(ifmIndex + 3, ifmIndex + 2 + length) which would negate the purpose of streams which go one by one.

• Also try using Arrow FP library (arrow-kt.io) – Ananth Raghuraman Jul 1 '18 at 18:16