Accounts Merge by tom4649 · Pull Request #108 · tom4649/Coding

tom4649 · 2026-05-23T22:43:06Z

https://leetcode.com/problems/accounts-merge/description/?envType=problem-list-v2&envId=rab78cw1

nodchip · 2026-05-29T10:18:57Z

+
+        name_to_indexes = {}
+        for i, account in enumerate(accounts):
+            name_to_indexes.setdefault(account[0], []).append(i)


同一の名前で異なるユーザーがいるにもかかわらず、同一のエントリーに追加している点に違和感を感じました。

見返して、このファイルのロジック自体が冗長だと思いました。
（このstep1.pyはLeetCode上で解くことだけを目的に書いたコードです。UnionFindを用いた改善をstep2_union_find.pyに書き、レビューを受けてその改善をstep2_union_find_revised.pyに行いました。）

nodchip · 2026-05-29T10:21:37Z

+                    [accounts[indexes[0]][0]] + sorted(set(accounts[indexes[0]][1:]))
+                ]
+
+            mail_to_rank = {}


rank が何を指しているのか分かりにくく感じました。これは indexes 内のインデックスでしょうか。より分かりやすい名前を付けるか、ロジック自体をシンプルにしたほうが良いと思いました。

nodchip · 2026-05-29T10:25:26Z

+                        account_union_find.union(r, mail_to_rank[email])
+                    else:
+                        mail_to_rank[email] = r
+            rank_to_index = {r: i for r, i in enumerate(indexes)}


これはそのまま indexes を使えばよいように思いました。 accounts[rank_to_index[r]][1:] は accounts[indexes[r]][1:] となると思います。

その通りで、rank_to_indexの作成自体不要ですね

nodchip · 2026-05-29T10:26:15Z

+
+        name_to_indexes = {}
+        for i, account in enumerate(accounts):
+            name_to_indexes.setdefault(account[0], []).append(i)


defaultdict を使用したほうがシンプルになると思いました。
https://docs.python.org/ja/3.6/library/collections.html#collections.defaultdict

nodchip · 2026-05-29T10:29:52Z

+                email_to_neighbor.setdefault(hub, []).append(email)
+                email_to_neighbor.setdefault(email, []).append(hub)
+
+        accounts_merged = []


merged_accounts のほうが語順的に自然に感じられます。

nodchip · 2026-05-29T10:33:12Z

+    def accountsMerge(self, accounts: List[List[str]]) -> List[List[str]]:
+        email_to_name = {}
+        email_to_neighbor = {}
+        hubs = []


hubs を定義せず、 email_to_name または email_to_neighbor のキーで traverse() を回したほうがシンプルになると思いました。

たしかに余計な変数でした。採用させていただきました。

nodchip · 2026-05-29T10:37:57Z

+    std::vector<int> parent;
+    std::vector<int> size;
+
+    UnionFind(int n) {


UnionFind(int n) : parent(n), size(n, 1) {

としたほうが良いかもしれません。詳しくは Effective C++ 第 3 版第１章４項「オブジェクトは、使う前に初期化しよう」をご覧ください。
https://www.maruzen-publishing.co.jp/book/b10111861.html

C++でメンバ変数は関数本体に入る前に初期化されるので、元のコードではデフォルトの状態でメンバ変数が作成された後、後からサイズや値が代入されることになっていた。
初期化子リストを使うと、無駄な空のインスタンス作成をスキップして、最初から必要な形でメモリを確保して作成される、ということですね。

紹介していただいた本は時々練習会で見かけるので気になっています。余裕ができたら手に取ってみようと思います。

nodchip · 2026-05-29T10:40:00Z

+            }
+        }
+
+        for (auto& [root, emails] : root_to_emails) {


非 const 参照で受けると、 root と emails を変更するという意図があると誤解させてしまうと思います。 const 参照で受けることをお勧めいたします。

そのような解釈をあたえることを認識していませんでした。修正いたしました。

nodchip · 2026-05-29T10:41:15Z

+    std::vector<std::vector<std::string>> accountsMerge(std::vector<std::vector<std::string>>& accounts) {
+        UnionFind union_find(accounts.size());
+        std::unordered_map<std::string, int> mail_to_account_index;
+        std::vector<std::vector<std::string>> accounts_merged;


宣言場所と使用箇所が離れていると、読み手は使用箇所まで変数を記憶しておかなければならず、読み手の短期記憶を圧迫してしまうと思います。使用箇所の直前で宣言することをお勧めいたします。

これは一般的な話ですね。なぜこの位置に書いたのか、自分でも分からずにいます。修正しました。

tom4649 added 2 commits May 24, 2026 07:42

721. Accounts Merge

73fe131

Add C++

a59399a

nodchip reviewed May 29, 2026

View reviewed changes

Add suggested changes

5d62400

Conversation

tom4649 commented May 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tom4649 May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tom4649 May 29, 2026 •

edited

Loading