refactor: extract pure computation helpers and correction functions#53
refactor: extract pure computation helpers and correction functions#53Wolfvin wants to merge 1 commit into
Conversation
This refactoring improves code organization without changing any behavioral output. Verified with 3-layer regression testing using the Regrets tool (all 6 clusters GREEN, all outputs identical). Changes: - Extract hitungDino/hitungPasaran pure helpers from silpin.ts (computes adjusted day/pasaran index without async wrapping) - Extract koreksiDino/koreksiPasaran to new koreksi.ts module (cycle wrapping functions for 7-day week and 5-day pasaran) - Move BATASAN_TYPE to types.ts as BatasanType (shared interface) - Export MIN_TAHUNJAWA/MAX_TAHUNJAWA constants from batur.ts - Add JSDoc documentation to all refactored functions Verification details in docs/REFACTOR_VERIFICATION.md
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR refactors a Javanese calendar library by extracting pure computation helpers, consolidating shared types, and reorganizing correction logic across modules. A new ChangesStructural refactoring with pure helpers and shared types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/koreksi.ts (1)
17-29: ⚡ Quick winAdd input validation to both correction helpers for robustness. Both
koreksiDinoandkoreksiPasaranlack validation for non-positive or non-integer inputs; passingd <= 0orp <= 0causes negative array indexing (DINTEN_ARR[-1]orPASARAN_ARR[-1]→undefined). While current callers initialize indices from.urutanproperties (≥ 1) and only increment, extracting these functions into a standalone module benefits from defensive programming.
src/koreksi.ts#L17-L29: validated >= 1andNumber.isInteger(d)before processing inkoreksiDino.src/koreksi.ts#L37-L51: validatep >= 1andNumber.isInteger(p)before processing inkoreksiPasaran.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/koreksi.ts` around lines 17 - 29, Add defensive input validation to both functions: in koreksiDino (src/koreksi.ts lines 17-29) and koreksiPasaran (src/koreksi.ts lines 37-51) verify that the incoming parameter is an integer >= 1 using Number.isInteger(...) and a >= 1 check before any modulo/index math; if the check fails, throw a clear TypeError (e.g., "koreksiDino: expected integer >= 1" / "koreksiPasaran: expected integer >= 1"). Keep the existing normalization logic (modulo 7 and modulo 5 respectively) and use the validated value to index DINTEN_ARR and PASARAN_ARR safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/REFACTOR_VERIFICATION.md`:
- Around line 99-107: Label the final fenced code block that contains the
test-summary lines (the block beginning with the lines like "✓
src/__tests__/_sasi.test.js ..." and ending with the three backticks) by
changing the opening/closing fence from ``` to ```text so the block is
explicitly marked as plain text and avoids MD040.
In `@src/index.ts`:
- Around line 4-5: Update the top-of-file comment to remove the incorrect claim
that hitungDino/hitungPasaran were imported into this file; keep the note that
koreksiDino/koreksiPasaran were extracted to koreksi.ts and, if desired, add a
brief clarifier that hitungDino and hitungPasaran were extracted to silpin.ts
(not imported here). Specifically edit the comment mentioning "imported
hitungDino/hitungPasaran from silpin.ts for reuse" so it either omits that
clause or replaces it with "hitungDino/hitungPasaran were extracted to silpin.ts
(not imported here)"; no code changes required.
In `@src/silpin.ts`:
- Around line 29-32: The index-wrapping logic in hitungDino (and the analogous
function handling pasaran at lines ~39-42) only subtracts one cycle and fails
for larger offsets; replace the conditional subtraction with full modulo
normalization using the correct modulus for each cycle (e.g., for hitungDino
compute normalized = ((dn + h - 1) % 7 + 7) % 7 and return that (or adjust +1/-1
if your indexing is 1-based), and for the pasaran function use modulus 5/6 as
appropriate) so indices always wrap into the valid range for lookups.
- Around line 117-123: The parameter type for cariWulanRegistry is incorrect:
change the signature from cariWulanRegistry(wulan: string): SasiReturnType to
accept both strings and numbers (e.g., cariWulanRegistry(wulan: string |
number): SasiReturnType), remove the unnecessary casts (like (wulan as unknown
as number)) and use wulan directly in the numeric branch to index SASI_ARR
(remember to subtract 1), and ensure any callers still pass string|number or are
updated to pass the correct type if needed.
---
Nitpick comments:
In `@src/koreksi.ts`:
- Around line 17-29: Add defensive input validation to both functions: in
koreksiDino (src/koreksi.ts lines 17-29) and koreksiPasaran (src/koreksi.ts
lines 37-51) verify that the incoming parameter is an integer >= 1 using
Number.isInteger(...) and a >= 1 check before any modulo/index math; if the
check fails, throw a clear TypeError (e.g., "koreksiDino: expected integer >= 1"
/ "koreksiPasaran: expected integer >= 1"). Keep the existing normalization
logic (modulo 7 and modulo 5 respectively) and use the validated value to index
DINTEN_ARR and PASARAN_ARR safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d0aa5a-3f24-479e-a7d6-d95a9bccc368
📒 Files selected for processing (6)
docs/REFACTOR_VERIFICATION.mdsrc/batur.tssrc/index.tssrc/koreksi.tssrc/silpin.tssrc/types.ts
| ``` | ||
| ✓ src/__tests__/_sasi.test.js (14 tests) | ||
| ✓ src/__tests__/_silpin.test.js (5 tests) | ||
| ✓ src/__tests__/_kurup.test.js (2 tests) | ||
| ✓ src/__tests__/_lib.test.js (1 test) | ||
| ... | ||
| Test Files 9 passed (9) | ||
| Tests 105 passed (105) | ||
| ``` |
There was a problem hiding this comment.
Label the final fenced block.
The closing code fence is unlabeled, which trips MD040 and makes the rendered output less consistent with the earlier fenced examples. Use a language tag such as text for the test-summary block.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/REFACTOR_VERIFICATION.md` around lines 99 - 107, Label the final fenced
code block that contains the test-summary lines (the block beginning with the
lines like "✓ src/__tests__/_sasi.test.js ..." and ending with the three
backticks) by changing the opening/closing fence from ``` to ```text so the
block is explicitly marked as plain text and avoids MD040.
Source: Linters/SAST tools
| // Refactored: extracted koreksiDino/koreksiPasaran to koreksi.ts, | ||
| // imported hitungDino/hitungPasaran from silpin.ts for reuse. |
There was a problem hiding this comment.
Fix misleading comment about imported functions.
Line 5 states "imported hitungDino/hitungPasaran from silpin.ts for reuse," but this file does not import those functions (see lines 15-21). According to the review stack context, hitungDino and hitungPasaran are extracted in silpin.ts (layer 5), not imported into index.ts. The comment should only reference the koreksiDino/koreksiPasaran extraction mentioned on line 4.
📝 Suggested correction
// Pustaka Kalender Jawa
// Perhtungan, pengkonversian dan penanggalan Jawa
// 📁 index.ts
-// Refactored: extracted koreksiDino/koreksiPasaran to koreksi.ts,
-// imported hitungDino/hitungPasaran from silpin.ts for reuse.
+// Refactored: extracted koreksiDino/koreksiPasaran to koreksi.ts for reuse.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Refactored: extracted koreksiDino/koreksiPasaran to koreksi.ts, | |
| // imported hitungDino/hitungPasaran from silpin.ts for reuse. | |
| // Pustaka Kalender Jawa | |
| // Perhtungan, pengkonversian dan penanggalan Jawa | |
| // 📁 index.ts | |
| // Refactored: extracted koreksiDino/koreksiPasaran to koreksi.ts for reuse. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 4 - 5, Update the top-of-file comment to remove
the incorrect claim that hitungDino/hitungPasaran were imported into this file;
keep the note that koreksiDino/koreksiPasaran were extracted to koreksi.ts and,
if desired, add a brief clarifier that hitungDino and hitungPasaran were
extracted to silpin.ts (not imported here). Specifically edit the comment
mentioning "imported hitungDino/hitungPasaran from silpin.ts for reuse" so it
either omits that clause or replaces it with "hitungDino/hitungPasaran were
extracted to silpin.ts (not imported here)"; no code changes required.
| function hitungDino(h: number, dn: number): number { | ||
| const raw = dn + h - 1; | ||
| return raw > 7 ? raw - 7 : raw; | ||
| } |
There was a problem hiding this comment.
Use full modulo normalization for index wrapping.
Line 31 and Line 41 subtract only one cycle. Larger offsets still produce out-of-range indices (e.g., day index > 7, pasaran index > 5), which can cause failed lookups.
Suggested patch
function hitungDino(h: number, dn: number): number {
const raw = dn + h - 1;
- return raw > 7 ? raw - 7 : raw;
+ return ((raw - 1) % 7 + 7) % 7 + 1;
}
function hitungPasaran(p: number, ps: number): number {
const raw = ps + p - 1;
- return raw > 5 ? raw - 5 : raw;
+ return ((raw - 1) % 5 + 5) % 5 + 1;
}Also applies to: 39-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/silpin.ts` around lines 29 - 32, The index-wrapping logic in hitungDino
(and the analogous function handling pasaran at lines ~39-42) only subtracts one
cycle and fails for larger offsets; replace the conditional subtraction with
full modulo normalization using the correct modulus for each cycle (e.g., for
hitungDino compute normalized = ((dn + h - 1) % 7 + 7) % 7 and return that (or
adjust +1/-1 if your indexing is 1-based), and for the pasaran function use
modulus 5/6 as appropriate) so indices always wrap into the valid range for
lookups.
| function cariWulanRegistry(wulan: string): SasiReturnType { | ||
| // console.log(ARANING_WULAN_SETAUN.get(Symbol.for(wulan))) | ||
| // cek wulan type number or string | ||
| let _wulan = ''; | ||
|
|
||
| if (typeof wulan === 'string') { | ||
| _wulan = wulan; | ||
| } else if (typeof wulan === 'number') { | ||
| const _sasi = SASI_ARR[wulan - 1]; | ||
| const _sasi = SASI_ARR[(wulan as unknown as number) - 1]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm current function signature and numeric branch:"
rg -nP 'function\s+cariWulanRegistry\s*\(\s*wulan\s*:\s*.*\)' src/silpin.ts
rg -nP 'typeof\s+wulan\s*===\s*[\"\x27]number[\"\x27]' src/silpin.ts
echo
echo "2) Check call sites passing numeric literals/variables:"
rg -nP '\bcariWulanRegistry\s*\(\s*\d+' src --type ts
rg -nP '\bcariWulanRegistry\s*\(\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\)' src --type tsRepository: kalenderjawa/pustaka
Length of output: 289
Fix cariWulanRegistry parameter type to match its runtime branches
In src/silpin.ts (lines 117–123), cariWulanRegistry is typed as wulan: string but still contains a typeof wulan === 'number' branch, making numeric handling unreachable for type-checked callers.
Suggested patch
-function cariWulanRegistry(wulan: string): SasiReturnType {
+function cariWulanRegistry(wulan: string | number): SasiReturnType {
let _wulan = '';
if (typeof wulan === 'string') {
_wulan = wulan;
} else if (typeof wulan === 'number') {
- const _sasi = SASI_ARR[(wulan as unknown as number) - 1];
+ const _sasi = SASI_ARR[wulan - 1];
if (_sasi) {
_wulan = _sasi.wulan;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function cariWulanRegistry(wulan: string): SasiReturnType { | |
| // console.log(ARANING_WULAN_SETAUN.get(Symbol.for(wulan))) | |
| // cek wulan type number or string | |
| let _wulan = ''; | |
| if (typeof wulan === 'string') { | |
| _wulan = wulan; | |
| } else if (typeof wulan === 'number') { | |
| const _sasi = SASI_ARR[wulan - 1]; | |
| const _sasi = SASI_ARR[(wulan as unknown as number) - 1]; | |
| function cariWulanRegistry(wulan: string | number): SasiReturnType { | |
| let _wulan = ''; | |
| if (typeof wulan === 'string') { | |
| _wulan = wulan; | |
| } else if (typeof wulan === 'number') { | |
| const _sasi = SASI_ARR[wulan - 1]; | |
| if (_sasi) { | |
| _wulan = _sasi.wulan; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/silpin.ts` around lines 117 - 123, The parameter type for
cariWulanRegistry is incorrect: change the signature from
cariWulanRegistry(wulan: string): SasiReturnType to accept both strings and
numbers (e.g., cariWulanRegistry(wulan: string | number): SasiReturnType),
remove the unnecessary casts (like (wulan as unknown as number)) and use wulan
directly in the numeric branch to index SASI_ARR (remember to subtract 1), and
ensure any callers still pass string|number or are updated to pass the correct
type if needed.
1 participant
Refactoring Summary
This PR introduces structural improvements to the Pustaka Kalender Jawa codebase without changing any behavioral output. All changes have been verified using a 3-layer regression testing approach.
Changes Made
1. Extracted Pure Computation Helpers (
silpin.ts)The
konversiHariandkonversiPasaranfunctions contained inline arithmetic mixed with async Promise wrapping. The pure computation has been extracted intohitungDino(h, dn)andhitungPasaran(p, ps)— small, testable, pure functions that compute the adjusted index. The async wrappers now delegate to these helpers.2. Extracted Correction Functions (
koreksi.ts)koreksiDinoandkoreksiPasaranwere inline helpers inindex.ts. Moved to dedicatedkoreksi.tsmodule with proper JSDoc documentation.3. Moved Interface to Shared Types
BATASAN_TYPErenamed toBatasanTypeand moved frombatur.tstotypes.ts, consistent with the project pattern.4. Exported Bounds Constants
MIN_TAHUNJAWAandMAX_TAHUNJAWAare now exported frombatur.ts.5. Improved Documentation
Added JSDoc comments explaining Javanese calendar concepts.
3-Layer Verification
All 6 behavioral clusters were captured before refactoring and validated after:
See
docs/REFACTOR_VERIFICATION.mdfor the full verification report.Summary by CodeRabbit
Refactor
Documentation