search

open-dis / open-dis-cpp

C++ implementation of the IEEE-1278.1 Distributed Interactive Simulation (DIS) application protocol v6 and v7
BSD 2-Clause "Simplified" License
90 stars 65 forks source link

Update OneByteChunk Class #80

Closed crhowell3 closed 1 year ago

crhowell3 commented 1 year ago

https://github.com/open-dis/open-dis-cpp/blob/6e14b5f8e3de9d55ea04083412284efa7e5a0fae/src/dis6/OneByteChunk.cpp#L30-L56

Why have character arrays that will only ever contain a single character? Why use character arrays at all? A single character would suffice. There is no need to have these arrays that are "iterated" over. Seems very arbitrary and overly complicated. Will probably create a PR to update this, unless there is a justifiable reason to leave it the way it is.

leif81 commented 1 year ago

I agree @crhowell3 . In the Java implementation we replaced all uses of OneByteChunk class with byte array for the same reason. I would be in favour of seeing the same done to the C++ implementation and would welcome a pull request!

See here for how it was done on the Java side, suggest doing something similar here. https://github.com/open-dis/open-dis-java/pull/44/files

crhowell3 commented 1 year ago

I will take a look at the Java side, and I'll have a PR ready soon. Thanks!

crhowell3 commented 1 year ago

I am considering applying Google Style Guide formatting to the code base for modern C++ style compliance, though I do not want to overstep my bounds. Any oppositions to this @leif81 ?

leif81 commented 1 year ago

Yes I'm good with that. Can you do the style change in a second PR after this one gets merged to make the review easier.