-
Notifications
You must be signed in to change notification settings - Fork 0
Description
The common package has accumulated helper-level tech debt (duplicate pointer helpers, redundant generic pointer helper, duplicated byte conversion logic, and inconsistent boolean encoding semantics). This makes the package harder to maintain and increases the risk of subtle behavioral inconsistencies.
This issue proposes a focused refactor to consolidate helpers, reduce duplication, and clarify semantics without changing externally visible behavior (except where existing behavior is clearly inconsistent/bug-prone).
Problems Identified
1) Duplicate pointer helpers
We currently maintain multiple type-specific pointer helpers:
Uint8Ptr,Uint16Ptr,Uint32PtrInt8PtrBoolPtrStringPtrFloat32Ptr,Float64PtrDurationPtr
Additionally, there is already a generic pointer helper:
DataRatePtr[T any](value T) *T
This is redundant and indicates the package is mid-transition.
2) Byte conversion helpers duplicate logic
UintToBytes and IntToBytes implement the same big-endian shift/write loop. Similarly, Float32ToBytes and Float64ToBytes duplicate the same loop pattern after converting to bits.
This increases maintenance burden and can lead to divergence.
3) Boolean encoding semantics are inconsistent
BoolToBytes(value bool, bit uint8)produces a bitmask (1<<bit) in a byte.BytesToBool(bytes []byte)checksbytes[0] == 0x01.
These semantics only align in the narrow case where bit == 0 and no other bits are present. This inconsistency can cause incorrect decode/encode behavior when bits other than 0 are used.
4) Minor tech debt / readability issues
TimePointeruses a local variable namedtimewhich shadows the importedtimepackage (readability issue).validateFieldValueconstructs a newvalidator.New()on each call (avoidable overhead).
Proposal
A) Introduce a single generic pointer helper and remove redundant helpers
Add:
func Ptr[T any](v T) *T { return &v }Then migrate call sites and remove:
Uint8Ptr,Uint16Ptr,Uint32PtrInt8PtrBoolPtrStringPtrFloat32Ptr,Float64PtrDurationPtrDataRatePtr(redundant oncePtr[T]exists)
B) Consolidate byte conversion logic
Refactor UintToBytes, IntToBytes, Float32ToBytes, Float64ToBytes to reduce duplication. Options:
- Introduce a shared internal helper for big-endian writing loops, or
- Use
encoding/binarywhere appropriate.
The key requirement is: preserve existing byte ordering and output length behavior.
C) Clarify boolean encoding and decoding semantics
Pick and document one of the following:
- Bit-flag semantics: provide a
BytesToBoolBit([]byte, bit)decoder and use it consistently withBoolToBytes(value, bit), OR - Byte-as-bool semantics: remove the
bitparameter and encode0x00/0x01, matchingBytesToBool.
This should be treated as a correctness fix (not merely style).
D) Minor readability/performance cleanups (optional but recommended)
- Rename the local variable in
TimePointerto avoid shadowingtime. - Reuse a package-level
validator.Validateinstance instead of constructing a new one per field.
Acceptance Criteria
- Add
Ptr[T any]helper - Remove
DataRatePtrand all type-specific*Ptrhelpers; migrate all call sites - Consolidate duplicated conversion logic for int/uint/float helpers (no behavioral regressions)
- Resolve
BoolToBytesvsBytesToBoolsemantic mismatch and update call sites accordingly - (Optional) Avoid
timepackage shadowing inTimePointer - (Optional) Reuse validator instance rather than
validator.New()per call - Unit tests updated/added for byte conversions and boolean semantics
Notes / Non-Goals
- This is not a redesign of the
Decode/Encodereflection model. - Focus is on helper simplification and reducing maintenance overhead while keeping behavior stable (except for boolean semantics, which currently appear inconsistent).