Update notebook metadata and improve documentation for Sleep, Cache, and Memoize#251
Merged
Conversation
## 修正概要 3つのLeetCode問題（2621 Sleep、2622 Cache With Time Limit、2623 Memoize）に対するレビュー指摘事項を修正 ## 2622. Cache With Time Limit - ipynb: メタデータをtypescriptに変更、cleanup()を1パス削除に最適化 - README: メモリリーク記述の正確化、ベンチマーク値に参考値明記 - README_react.html: React本番ビルド使用、自動再生間隔を定数化 ## 2623. Memoize - ipynb: メタデータ修正、キー衝突対策、二重検索回避 - README: 文法・用語修正、可変長関数の注意書き追加 ## 2621. Sleep - ipynb: メタデータ修正、バリデーション順序改善 - README: 非推奨API修正、マイクロ最適化主張の見直し
## 修正内容 ### Space複雑度の正確化 - cleanup()はインプレース削除のため Space: O(n) → O(1) に修正 - CacheWithTimeLimit_TS.ipynb - README.md (2622) ### Pythonバージョン情報の修正 - asyncio.get_running_loop() は Python 3.7+ で利用可能 - コメントを "3.10+推奨" → "3.7+で推奨" に修正 - README.md (2621) ### 可読性優先の実装変更 - cache.has() + cache.get()! パターンに戻す (2箇所) - パフォーマンスより明示性を優先 - Memoize_TS.ipynb
Contributor
📝 Walkthroughウォークスルー複数のノートブックのメタデータをPythonからTypeScriptに更新し、Sleep実装のasyncio APIを最新化、キャッシュ有効期限戦略（怠惰削除とアクティブ削除）の説明を明確化、複数ファイルにおける用語と制約を整理しました。 変更内容
推定コードレビュー労力🎯 3 (中程度) | ⏱️ ~25分 関連PRの可能性
ウサギからの詩
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@JavaScript/2623`. Memoize/Claude Code Sonnet 4.5/Memoize_TS.ipynb:
- Around line 130-142: The memoize implementation incorrectly compresses keys
assuming 1–2 numeric args (in function memoize, cache Map and memoized), causing
collisions for 3+ args; change the key-generation to handle variable-length
args: keep the numeric compression for 1 or 2 args if you want that
optimization, but when args.length > 2 (or when any arg is non-number) fall back
to a safe generic key (e.g., stringified args via JSON.stringify or args.join
with a separator) and update the cache Map type accordingly (e.g., Map<string |
number, number> or unify to Map<string, number>) so all keys map correctly
without collisions. Ensure memoized uses that unified key strategy for all
calls.
In `@JavaScript/2623`. Memoize/Claude Code Sonnet 4.5/README.md:
- Around line 38-41: The README's fixed-arity memoize example lacks an
argument-count guard, allowing mixed arity calls to produce NaN/compressed-key
collisions; update the sample for the memoize implementation to explicitly
validate the number of arguments (e.g., check args.length or the expected arity
at the start of the memoized wrapper) and either throw a clear error or handle
mismatched arity before computing keys (particularly before using the compressed
key formula `a * 100001 + b`), so callers cannot produce NaN keys or mix 1- and
2-argument invocations.
Comment on lines
+130
to
+142
| "function memoize(fn: Fn): Fn {\n", | ||
| " // sum: 引数2つ(a, b) → a * 100001 + b で一意な整数キーに圧縮\n", | ||
| " // fib/factorial: 引数1つ(n) → nそのもの\n", | ||
| " // 両方対応するため、ネスト Map を使用しない。\n", | ||
| " // 引数数で分岐し、数値キーのみで Map を構築する。\n", | ||
| " const cache = new Map<number, number>();\n", | ||
| " let callCount = 0;\n", | ||
| "\n", | ||
| " const memoized: Fn = function (...args: number[]): number {\n", | ||
| " // 引数が1つなら n そのもの、2つなら圧縮キー\n", | ||
| " const key = args.length === 1\n", | ||
| " ? args[0]\n", | ||
| " : args[0] * 100001 + args[1];\n", |
Contributor
There was a problem hiding this comment.
可変長引数に非対応なキー圧縮は誤解を招きます
memoize のシグネチャは可変長引数ですが、この最適化は 1～2 引数前提で args[2] 以降を無視するため、3 つ以上の引数で衝突し誤ったキャッシュヒットになります。最適化を残すなら「1〜2 引数限定」を明記するか、超過時に汎用キーへフォールバックするなどのガードが必要です。
🛠️ 修正案（可変長引数は汎用キーにフォールバック）
- const cache = new Map<number, number>();
+ const cache = new Map<string, number>();
@@
- const key = args.length === 1
- ? args[0]
- : args[0] * 100001 + args[1];
+ const key =
+ args.length === 1
+ ? `n:${args[0]}`
+ : args.length === 2
+ ? `s:${args[0] * 100001 + args[1]}`
+ : `v:${args.join(",")}`;🤖 Prompt for AI Agents
In `@JavaScript/2623`. Memoize/Claude Code Sonnet 4.5/Memoize_TS.ipynb around
lines 130 - 142, The memoize implementation incorrectly compresses keys assuming
1–2 numeric args (in function memoize, cache Map and memoized), causing
collisions for 3+ args; change the key-generation to handle variable-length
args: keep the numeric compression for 1 or 2 args if you want that
optimization, but when args.length > 2 (or when any arg is non-number) fall back
to a safe generic key (e.g., stringified args via JSON.stringify or args.join
with a separator) and update the cache Map type accordingly (e.g., Map<string |
number, number> or unify to Map<string, number>) so all keys map correctly
without collisions. Ensure memoized uses that unified key strategy for all
calls.
Contributor
There was a problem hiding this comment.
No description provided.
Comment on lines
+38
to
+41
| - **キー設計（固定アリティ前提）**: | ||
| - 引数1つの場合: キー = `n` そのもの | ||
| - 引数2つの場合: キー = `a * 100001 + b`（圧縮キー） | ||
| - ※ 同一 memoize で可変長（1/2引数混在）を許すとキー衡突が起こり得るため不可 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
固定アリティ前提なら、実装サンプルにも引数数チェックを明記して整合性を上げると良いです。
TL;DR で固定アリティを強調している一方、コード例にはガードがなく、誤った呼び出しで NaN キー化する余地があります。README 内のサンプルに軽いチェックを入れると意図が明確になります。
🔧 追記例（README のコードブロック内）
- const key = args.length === 1 ? args[0] : args[0] * 100001 + args[1];
+ if (args.length !== 1 && args.length !== 2) {
+ throw new Error("memoize expects 1 or 2 numeric arguments.");
+ }
+ const key = args.length === 1 ? args[0] : args[0] * 100001 + args[1];🤖 Prompt for AI Agents
In `@JavaScript/2623`. Memoize/Claude Code Sonnet 4.5/README.md around lines 38 -
41, The README's fixed-arity memoize example lacks an argument-count guard,
allowing mixed arity calls to produce NaN/compressed-key collisions; update the
sample for the memoize implementation to explicitly validate the number of
arguments (e.g., check args.length or the expected arity at the start of the
memoized wrapper) and either throw a clear error or handle mismatched arity
before computing keys (particularly before using the compressed key formula `a *
100001 + b`), so callers cannot produce NaN keys or mix 1- and 2-argument
invocations.
myoshi2891
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.