refactor: make identifier logic more readable, rename faulty named function #3843
Conversation
📝 WalkthroughWalkthroughReplaces mod11-based check-digit logic in OrganisationLookup and PersonLookup validation modules with a modular additive-inverse approach; updates digit extraction and weight application. Public function names and signatures are unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
HauklandJ
added
kind/other
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/layout/OrganisationLookup/validation.ts (1)
58-58: Duplicate function definition across files.This is the same
modular_additive_inversefunction defined in PersonLookup/validation.ts. See the comment in that file for recommendations on extracting to a shared utility module.
🧹 Nitpick comments (3)
src/layout/PersonLookup/validation.ts (2)
50-51: The two-step modulo operation obscures intent.The pattern of calling
modular_additive_inverse()followed by% 11suggests the helper function doesn't fully implement the modular additive inverse. This makes the code less clear.Consider incorporating the final modulo operation into the helper function:
- let calculated_k1 = modular_additive_inverse(sum1, 11); - calculated_k1 = calculated_k1 % 11; + const calculated_k1 = modular_additive_inverse(sum1, 11) % 11;Or refactor the helper to return values in the range [0, base-1]:
const modular_additive_inverse = (value: number, base: number): number => { const remainder = value % base; return remainder === 0 ? 0 : base - remainder; };
64-65: Same two-step modulo pattern as K1 calculation.Same issue as lines 50-51. Consider the same refactor.
src/layout/OrganisationLookup/validation.ts (1)
50-51: Same two-step modulo pattern as PersonLookup.Same pattern as in PersonLookup/validation.ts. Refer to the recommendations in that file for consolidating this into a single operation or refactoring the helper function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/layout/OrganisationLookup/validation.ts(1 hunks)src/layout/PersonLookup/validation.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/OrganisationLookup/validation.tssrc/layout/PersonLookup/validation.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (1)
src/layout/OrganisationLookup/validation.ts (1)
45-48: Readability improvement from explicit loop.The explicit loop over weights is clearer and more maintainable than the previous implementation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/layout/OrganisationLookup/validation.ts (1)
40-41: Add digit validation to prevent NaN in calculations.The past review comment correctly identified that the length check doesn't validate that all characters are digits. If
orgNrcontains non-numeric characters,map(Number)will produceNaNvalues, leading to incorrect calculations.As suggested in the previous review, consider adding digit validation:
- if (orgNr.length !== 9) { + if (orgNr.length !== 9 || !/^\d{9}$/.test(orgNr)) { return false; }
🧹 Nitpick comments (3)
src/layout/OrganisationLookup/validation.ts (3)
43-48: LGTM: Clear and correct weighted sum calculation.The weights array and loop-based sum calculation are correct and readable. The refactor improves clarity over the previous approach.
Optionally, you could use a more functional style with
reduce:const sum = orgnr_digits.slice(0, 8).reduce((acc, digit, i) => acc + digit * weights[i], 0);However, the current imperative approach is perfectly fine and may be more readable to some developers.
50-51: Consider simplifying the two-step modulo calculation.The calculation is correct, but applying modulo twice (once in the helper, once explicitly) is slightly unclear. The standard mod 11 check digit formula is typically written as a single expression.
You could simplify by inlining:
- let calculated_k1 = modular_additive_inverse(sum, 11); - calculated_k1 = calculated_k1 % 11; + const calculated_k1 = (11 - (sum % 11)) % 11;Or keep the helper but make it include the final modulo operation (see comment on line 58).
58-58: Consider including the final modulo in the helper function.The function name
modular_additive_inversesuggests a complete modular arithmetic operation, but the implementation returnsbase - (value % base)without the final modulo. Whenvalue % base == 0, this returnsbaseinstead of0, requiring the caller to apply% 11again.You could make the helper self-contained:
-const modular_additive_inverse = (value: number, base: number): number => base - (value % base); +const modular_additive_inverse = (value: number, base: number): number => (base - (value % base)) % base;This would eliminate the need for the additional modulo operation on line 51.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/layout/OrganisationLookup/validation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/OrganisationLookup/validation.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Install
|
2 participants
Description
For a discussion of the name of the function, see https://math.stackexchange.com/questions/5107917/is-there-a-term-for-a-xmoda
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit