Skip to content

Conversation

@Fuminiton
Copy link
Owner

return None
root_val = next(preorder_val)
root = TreeNode(root_val)
deviding_position = val_to_index[root_val]

Choose a reason for hiding this comment

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

dividingではないでしょうか。

Copy link
Owner Author

Choose a reason for hiding this comment

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

コメントありがとうございます。
dividing_positionが正しいです。


### 感想
- `val_to_index`より`val_to_inorder_index`とかの方が親切
- `iter(preorder)`の部分は`nonlocal`でpreprderのインデックスをもって管理する方が多数派だった
Copy link

Choose a reason for hiding this comment

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

私は、iter next でやるのにはあまり賛成しないです。結局のところ、メソッドが呼ばれた順番の整合を取る必要があるのです。build_binary_tree_helper が何文字使ったかを返す、くらいでもいいと思います。

nonlocal もそういう意味では同じですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@oda
レビューありがとうございます。

結局のところ、メソッドが呼ばれた順番の整合を取る必要があるのです。

ここの意図の確認なのですが、

  • 何番目にメソッドが呼ばれているかが追えないために、編集がしにくい
  • バグや入力に不正があったときにデバッグしにくい

のような状況を避けるためといった理解であいますでしょうか?

Copy link

Choose a reason for hiding this comment

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

そうですね。

データの整合性が取れているということは、読んでいる人からすると全部読み終わらないと分からないからです。

https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.ho7q4rvwsa1g

これと同じ話です。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。
自分はこの観点を意識したコードの読み書きができていなそうなので、今後気をつけます。

if not left <= right:
return None
node_val = next(preorder_iterator)
deviding_index = inorder_val_to_index[node_val]

Choose a reason for hiding this comment

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

=の右側見ても分からなくはないですが、何をdivideするインデックスなのか書くとより親切かなと感じます。
また、もっと細かい点ですが、splitの方が今回は表現として適しているかもしれません。
https://ub-english.com/blog1346/

Copy link
Owner Author

Choose a reason for hiding this comment

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

コメントありがとうございます。
目に沿って左と右に分けるというイメージのsplitの方が適切ですね。

改めて考えましたが、ここ単にinorder_indexとした方がいい気がしてきました。

node_val = next(preorder_iter)
deviding_index = value_to_index[node_val]
node.val = node_val
if deviding_index + 1 <= right:
Copy link

Choose a reason for hiding this comment

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

この条件が何か一瞬わかりませんでした。
変数に入れてもよいかもしれないです。

Copy link

Choose a reason for hiding this comment

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

一旦スタックにいれて出すときに判断しても良いですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

レビューありがとうございます。

一旦スタックにいれて出すときに判断しても良いですね。

おっしゃる通りですね。

```

##### ループ
- `if left > right`の部分を分解してstackに入れる段階で弾くようにしているので、完全な書き換えではなさそう。
Copy link

Choose a reason for hiding this comment

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

あと、上では.leftと.rightを帰りがけに繋げているのに対し、下では行きがけに繋げていますね

val_to_index[val] = index
preorder_val = iter(preorder)

def build_binary_tree_helper(left: int, right: int
Copy link

Choose a reason for hiding this comment

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

おそらくかなり個人的なものの好みですが、[left, right)の方が自然な感じがします。(build_binary_tree_helper(0, len(preorder) - 1)ではなく、build_binary_tree_helper(0, len(preorder))になるようにする)
Pythonの配列のスライスがそうだからかもしれません

Copy link
Owner Author

Choose a reason for hiding this comment

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

レビューありがとうございます。

Pythonの配列のスライスがそうだからかもしれません

この説明かなり腑に落ちました。ありがとうございます。

Comment on lines +144 to +146
node_val = next(preorder_iter)
deviding_index = value_to_index[node_val]
node.val = node_val
Copy link

Choose a reason for hiding this comment

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

node.val = next(preorder_iter)
deviding_index = value_to_index[node.val]

ではダメですか?

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.

7 participants