Skip to content

【Arai60】23問目 617_Merge Two Binary Trees#23

Merged
shining-ai merged 5 commits intomainfrom
review23
Jun 30, 2024
Merged

【Arai60】23問目 617_Merge Two Binary Trees#23
shining-ai merged 5 commits intomainfrom
review23

Conversation

@shining-ai
Copy link
Copy Markdown
Owner

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.

非破壊的にしたい場合自分なら再帰の中でdeepcopyしちゃうと思います。

if not root1:
   return copy.deepcopy(root2)

あとは個人的にはFB反映はlevel_4.pyなどと新しいファイルでコミットしてもらったほうがレビューしやすいです。(あくまでも個人的な感想)

if not node1 and not node2:
return None
if not node1:
return node2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

root2をルートとするツリーについては、引数で渡ってきたものをそのまま使用しているので、この書き方だとmergeTreesの返り値のTreeにroot2の一部が使われてしまいます。この関数自体では非破壊的かもしれないですが、mergeTreesの返り値のTreeの操作を行うとroot2のツリーを書き換えてしまう可能性があります。

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.

返り値のTreeが中途半端でたちが悪くなっていることに気づきませんでした。
戻り値にdeepcopyが必要ですね。

node1.right = helper_merge_tree(node1.right, node2.right)
return node1

sum_tree = deepcopy(root1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sum_treeの命名がなんか気になります。merged_treeのほうが良いかなと思います。あと実際には、これはマージしたツリーではなく単にroot1のコピーなのでroot1_cloneとかですかね。

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.

コピーした時点ではmerged_treeではないので命名が良くないことは、とても参考になりました。

self, root1: Optional[TreeNode], root2: Optional[TreeNode]
) -> Optional[TreeNode]:
def helper_merge_trees(node1, node2):
if not node1 and not node2:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

このif文はなくても動くと思います。

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文でしたので削除します。
解法が整理しきれてませんでしたね。

nodes_queue = deque()
sum_tree = deepcopy(root1)
sum_tree, nodes_queue = merge_nodes(sum_tree, root2, nodes_queue)
while nodes_queue:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

なんかものすごく読みづらいんですよね。queueを引数で取り回しているからですかね?

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.

確かに変な書き方になっている気がします。
helper_merge_treesを呼ぶと気づいたらnodes_queueに追加されていて利用者がびっくりすることを避けたかったんですよね。
イマイチなのは分かるんですが、直し方がわからないですね...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

メインのwhileループで1番気になるのが、queueのライフサイクル(いつ取り出されていつ何が入ってくるのか)だと思うんですけど、関数の中でやってしまうとそこが追いづらいのがあるのかなと思います。

うーん、書いてみたんですけどあんまり良くなっていないかもです。二分木で親と子の繋ぎ変えが発生する場合はBFSだと書きにくいですね。

class Solution:
    def mergeTrees(self, root1: Optional[TreeNode], root2: Optional[TreeNode]) -> Optional[TreeNode]:
        def merge_nodes(node1, node2):
            if not node1:
                return copy.deepcopy(node2)
            if not node2:
                return node1
            node1.val += node2.val
            return node1

        root1_clone = copy.deepcopy(root1)
        merged_tree_root = merge_nodes(root1_clone, root2)
        queue = deque()
        if root1_clone and root2:
            queue.append((root1_clone, root2))
        while queue:
            node1, node2 = queue.popleft()
            # 本当はqueueへの追加と左右のマージの処理の順番は逆にしたいがnode1.left, node1.rightを書き換えるのでこの順番でないといけない。
            if node1.left and node2.left:
                queue.append((node1.left, node2.left))
            if node1.right and node2.right:
                queue.append((node1.right, node2.right))
            node1.left = merge_nodes(node1.left, node2.left)
            node1.right = merge_nodes(node1.right, node2.right)
        return merged_tree_root

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.

ありがとうございます。
関数外でqueueの追加をするこちらの方がベターだと思いました。

# DFS(非破壊)
# レビューコメントを反映
class Solution:
def mergeTrees(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ヘルパー関数なしでも書けそうですね。

@hayashi-ay
Copy link
Copy Markdown

良いと思います。

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.

2 participants