4
\$\begingroup\$

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++
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Can you add some more context, what exactly is CanFrame ? \$\endgroup\$ – Simon Forsberg Jun 21 '18 at 14:08
  • \$\begingroup\$ 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 \$\endgroup\$ – Ernest Zamelczyk Jun 21 '18 at 14:13
  • \$\begingroup\$ I'd recommend you to read Simon's guide to a good question \$\endgroup\$ – Simon Forsberg Jun 21 '18 at 16:20
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.