Skip to content

2. Add Two Numbers#21

Merged
colorbox merged 3 commits intomainfrom
2
Nov 12, 2024
Merged

2. Add Two Numbers#21
colorbox merged 3 commits intomainfrom
2

Conversation

@colorbox
Copy link
Copy Markdown
Owner

Comment on lines +19 to +26
if (carry) {
sum++;
carry = false;
}
if (sum>=10) {
sum -= 10;
carry = true;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carryをintで持つようにするとこの辺りの分岐は全部消せそうです。carryの更新は carry = sum / 10; で出来ます。
あと0と1以外も扱えるようになるため、応用範囲も広がると思います。(この問題では0か1しか出ませんが)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます。
carryフラグに引っ張られすぎていました、intで保持して更新が楽になる方が良さそうですね

class Solution {
public:
ListNode* addTwoNumbers(ListNode* l1, ListNode* l2) {
ListNode sentinel_node = ListNode();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListNode sentinel_node; で良さそうです。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます、これはたしかにそのとおりですね。

public:
ListNode* addTwoNumbers(ListNode* l1, ListNode* l2) {
ListNode sentinel_node = ListNode();
ListNode* sum_node = &sentinel_node;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

もっと良い命名がありそうです。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと考え直します。

Comment on lines +6 to +7
ListNode* left = l1;
ListNode* right = l2;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あまりこの詰替えがコードの分かりやすさに貢献していない気がします。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます。
引数として渡されてきているl1,l2を書き換えたくなかったので詰め替えていました。
変数名を改善すると多少マシになりそうですかね

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

書き換えたところで呼び出し元に影響があるわけではないのでただイテレートするだけなら書き換えちゃって問題ないかなと思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discord内の引数に関する議論を一通り眺めましたが、この場合はl1,l2をそのまま使っても自然な気がしますね
ちょっとその方針でも一度書いてみます。

ListNode* left = l1;
ListNode* right = l2;
bool carry =false;
while (left || right) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自分ならcarryもこのwhileの条件で見てしまうと思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。
なるほど、確かにその方が余計な処理をwhileループの外に書かずにすみますね。

ListNode* sum_node = &sentinel_node;
ListNode* left = l1;
ListNode* right = l2;
bool carry =false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

細かいですが = の左右は1つスペースを挟むのが通常だと思います

sum++;
carry = false;
}
if (sum>=10) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/colorbox/leetcode/pull/21/files#r1679006461

同上です
なにかフォーマッタできれいにしてからpushしたほうが良さそうです

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あとスタイルガイドに目を通したり、同じ言語で実装された著名なライブラリを眺めてみるといいかもと思いました。演算子の両端に半角をスペースを入れる、といったことは慣習として大体どの言語の実装でもやることなので、時間を取ってコードを読む時間を設ければ、自然と違和感を感じれるようになってくると思います。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この問題を解いた別の方のプルリクエストなんかも、気軽に読める良い題材だと思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます。
これはtypoというかコードセルフチェックがちゃんとできていないですね。
読み直す時間取るのとフォーマッタ導入をします。

Copy link
Copy Markdown

@hayashi-ay hayashi-ay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step3に比べると読みやすくなったと思います。

public:
ListNode* addTwoNumbers(ListNode* l1, ListNode* l2) {
ListNode sentinel_node;
ListNode* adding_node = &sentinel_node;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

もう少し良い命名がある気もします。ものとしては1つ前の桁について持っているので、自分ならprevとかにするかなと思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほどありがとうございます。
加算を行うノードとして捉えてましたが、prevとしても通じますね

digit += l2->val;
l2 = l2->next;
}
if (carry) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carryを0, 1で持っているのでif文なしでdigit += carryと書けます

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。
確かに分岐なくすことでシンプルにできますね

if (digit >= 10) {
digit = digit - 10;
}
adding_node->next = new ListNode(digit);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここでdigit % 10としても良いかなと思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど、ありがとうございます。
%にすれば桁上りしててもしてなくても関係なく扱えますね

Copy link
Copy Markdown

@hayashi-ay hayashi-ay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

だいぶ読みやすくなったように思います。

@rihib rihib mentioned this pull request Aug 6, 2024
@colorbox colorbox merged commit bd1e242 into main Nov 12, 2024
@colorbox colorbox deleted the 2 branch November 12, 2024 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants