-
Notifications
You must be signed in to change notification settings - Fork 0
206. Reverse Linked List #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ryosuketc
wants to merge
1
commit into
main
Choose a base branch
from
206_reverse_linked_list
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| # 206. Reverse Linked List | ||
|
|
||
| https://leetcode.com/problems/reverse-linked-list/ | ||
|
|
||
| ## Comments | ||
|
|
||
| ### step1 | ||
|
|
||
| * まあ典型的な問題なのでさらっと。 | ||
| * 久々に再開したので `ListNode*` (ポインタ) を書くべきところを `ListNode` にしていた | ||
|
|
||
| ### step2 | ||
|
|
||
| * `nullptr` との比較 (while の条件) は暗黙評価でよかった。 | ||
| * 前にも聞いたことあったけど Gemini に聞いてみた | ||
|
|
||
| > 3 の `!= nullptr` についての指摘ですが、たとえば Python などでは、暗黙に bool に変換するよりも `is None` / `is not None` など明示的に `None` と比較するほうがよいとされています (Google Python Style Guide など)。なぜ C++ では暗黙に評価するほうがよいのですか。 | ||
|
|
||
| これは非常に的を射た指摘です。この違いは、まさに Python と C++ の設計思想(Zen)の違いを象徴しています。 | ||
|
|
||
| あなたが言う通り、Python の "Zen" (PEP 20) には「Explicit is better than implicit.(暗黙的より明示的が良い)」という有名な一文があります。is not None は、この哲学を完璧に体現しています。 | ||
|
|
||
| ではなぜ C++ では while (node) のような暗黙的な評価が好まれるのか。理由は主に2つあります。 | ||
|
|
||
| 1. C 言語から受け継いだ「慣習(イディオム)」 | ||
|
|
||
| C++ のこの文化は、その前身である C 言語から直接受け継がれています。 | ||
|
|
||
| C 言語の「NULL」は「0」だった: | ||
|
|
||
| C 言語には bool 型がなく、0 を「偽」、それ以外(非ゼロ)を「真」として扱いました。NULL マクロは、一般的に 0(または (void*)0)として定義されていました。 | ||
|
|
||
| 「ポインタ != 0」がイディオムになった: | ||
|
|
||
| そのため、while (ptr != 0)(つまり while (ptr != NULL))と書くよりも、単に while (ptr) と書く方が「ptr が 0 (NULL) でない間」という意味で簡潔かつ直接的でした。 | ||
|
|
||
| C++ への継承: | ||
|
|
||
| C++ はこの慣習をそのまま引き継ぎました。C++11 で型安全な nullptr が導入されましたが、nullptr も bool に変換される際は false として評価されます。したがって、while (node) という書き方は、C/C++ プログラマにとって数十年にわたる「ポインタが null でないことを確認する」ための標準的なイディオム(慣用句)であり続けています。 | ||
|
|
||
| 2. Google Style Guide の「例外」 | ||
|
|
||
| あなたの指摘は、実は Google のスタイルガイド内部の比較においても非常に興味深い点です。 | ||
|
|
||
| Python (Explicit): | ||
|
|
||
| Google Python Style Guide は if x: ではなく if x is not None: を推奨します。これは「空のリスト []」と None を区別するなど、明示性を重んじるためです。 | ||
|
|
||
| C++ (Implicit): | ||
|
|
||
| 一方、Google C++ Style Guide は、ポインタに関しては Python とは逆の方針を推奨しています。 | ||
|
|
||
| Null, 0, and '' | ||
|
|
||
| ... | ||
|
|
||
| Use if (ptr) and if (!ptr) for nullptr checks. | ||
|
|
||
| (訳: nullptr のチェックには if (ptr) と if (!ptr) を使いなさい。) | ||
|
|
||
| Google という同じ組織が、言語ごとに異なる(一見、矛盾するような)ガイドラインを設けているのです。これは、while (node) が C++ のコミュニティにおいていかに「ネイティブな」表現であるかを強く裏付けています。C++ においては、while (node != nullptr) と書くほうが、むしろ冗長で「C++ らしくない」と見なされる傾向があります。 | ||
|
|
||
| まとめ | ||
|
|
||
| Python: 「明示的 is 善」。is not None | ||
|
|
||
| C++: 「C からの伝統的なイディオムと簡潔性 is 善」。while (node) | ||
|
|
||
| 私のレビューは、この C++ の強い慣習に基づいています。!= nullptr と書くことは、コードの正しさには何の影響もありませんが、C++ の「流儀」からは少し外れている、と見なされることが多いのです。 | ||
|
|
||
| ### step3 | ||
|
|
||
| * 典型問題すぎるので省略 |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /** | ||
| * Definition for singly-linked list. | ||
| * struct ListNode { | ||
| * int val; | ||
| * ListNode *next; | ||
| * ListNode() : val(0), next(nullptr) {} | ||
| * ListNode(int x) : val(x), next(nullptr) {} | ||
| * ListNode(int x, ListNode *next) : val(x), next(next) {} | ||
| * }; | ||
| */ | ||
| class Solution { | ||
| public: | ||
| ListNode* reverseList(ListNode* head) { | ||
| ListNode* previous_node = nullptr; | ||
| ListNode* node = head; | ||
| while (node != nullptr) { | ||
| ListNode* next_node = node->next; | ||
| node->next = previous_node; | ||
| previous_node = node; | ||
| node = next_node; | ||
| } | ||
| return previous_node; | ||
| } | ||
| }; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /** | ||
| * Definition for singly-linked list. | ||
| * struct ListNode { | ||
| * int val; | ||
| * ListNode *next; | ||
| * ListNode() : val(0), next(nullptr) {} | ||
| * ListNode(int x) : val(x), next(nullptr) {} | ||
| * ListNode(int x, ListNode *next) : val(x), next(next) {} | ||
| * }; | ||
| */ | ||
| class Solution { | ||
| public: | ||
| ListNode* reverseList(ListNode* head) { | ||
| ListNode* previous_node = nullptr; | ||
| ListNode* node = head; | ||
| while (node) { | ||
| ListNode* next_node = node->next; | ||
| node->next = previous_node; | ||
| previous_node = node; | ||
| node = next_node; | ||
| } | ||
| return previous_node; | ||
| } | ||
| }; |
Empty file.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gemini に投げてみたらまあまあ怒られた (nullptr の件はこれで気づいて修正した)
このコードは、アルゴリズムとしては標準的で効率的ですが、コードの品質という観点からは多くの問題点を指摘できます。
変数名が不適切かつ一貫性に欠ける
nodeという変数名は、情報量がゼロに等しい最悪の命名です。リンクリストを扱うコードでは、ほとんどの変数がノード(またはノードへのポインタ)を指します。nodeという名前は、それが「どの」ノードなのか全く示していません。currentやcurrといった、その役割を明確に示す名前を使うべきです。previous_nodeとnext_nodeでは_nodeというサフィックスを使っているのに、中心となるはずのnodeにはそれがないため、命名規則に一貫性がありません。prev,curr,nextのように簡潔に統一するか、previousNode,currentNode,nextNodeのように(C++ではキャメルケースやスネークケースが好まれますが)スタイルを統一すべきです。説明責任の放棄(コメントの欠如)
node->nextを上書きすると、次のノードへの参照が失われるため)、 のようなポインタの移動がどのように行われるのか、コメントが一切ありません。C++の慣習に対する無頓着
while (node != nullptr)という条件は冗長です。C++ではポインタ型が暗黙的にboolに変換されるため(nullptrであればfalse、それ以外はtrue)、while (node)と書く方が簡潔であり、より C++ らしい書き方です。!= nullptrと明記するのは、時として可読性を助ける場合もありますが、この文脈では冗長に映ります。エッジケースの考慮がコードから読み取れない
headがnullptr(空のリスト)の場合や、要素が1つの場合でも正しく動作します。headがnullptrの場合にnodeもnullptrになり、即座にループが終了してprevious_node(nullptr)が返る、という流れがコードから直感的に読み取れません。if (head == nullptr || head->next == nullptr)のようなガード節(早期リターン)を追加する方が、コードの意図が明確になり、読み手も安心して主要なロジック(ループ部分)を読むことができます。総じて、このコードは「ただ動くだけ」のレベルであり、他者(あるいは未来の自分)が容易に理解し、保守できるような「良いコード」とは言えません。