Skip to content

103. binary tree zigzag level order traversal#28

Open
5ky7 wants to merge 2 commits intomainfrom
103.-Binary-Tree-Zigzag-Level-Order-Traversal
Open

103. binary tree zigzag level order traversal#28
5ky7 wants to merge 2 commits intomainfrom
103.-Binary-Tree-Zigzag-Level-Order-Traversal

Conversation

@5ky7
Copy link
Owner

@5ky7 5ky7 commented Dec 20, 2025

* Step1で言及した,`deque`から`vector`への変換のところでreverseを利用
* `GetVectorFromDeque()`の計算量は,`len(deque) / 2`回 swap するのでO(len(deque))だけ増える.
* しかし元々O(len(deque))なので結局定数倍のみ..
* [`std::view::reverse`](https://cpprefjp.github.io/reference/ranges/reverse_view.html)の利用.[Code3](#Code3)
Copy link

Choose a reason for hiding this comment

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

C++20 からみたいですね。たまに調べておきましょう。

public:
vector<vector<int>> zigzagLevelOrder(TreeNode* root) {
vector<vector<int>> result;
if (!root) {
Copy link

Choose a reason for hiding this comment

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

後続の処理に使用する変数を定義してから特別な入力に対する early return を行うと、やや短期記憶が圧迫される感じがします。 early return をしてから、変数を定義したほうが良いと思います。

return result;
}

std::deque<TreeNode*> frontiers;
Copy link

Choose a reason for hiding this comment

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

複数形の s を付けている点に違和感を感じました。あくまで個人の解釈なのですが、 frontier は未探索の領域全体を表し、その中に複数の要素が含まれるように感じます。この解釈を基にするのであれば、 frontier 自体は単数とし、その中に複数の要素を含めるのが良いと思います。

}

std::deque<TreeNode*> frontiers;
std::deque<TreeNode*> next_frontiers;
Copy link

Choose a reason for hiding this comment

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

next_frontiers はループ内で宣言し、ループの最後で frontiers に move すると、ループ外の変数が減り、少しシンプルになると思います。この場合、 root は frontiers のほうに push_back() することになり、 while 文の条件は frontiers に対して行うことになると思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

実は初めはnext_frontiersをループ内で定義していたのですが,外側のwhileと内側のwhileの条件部が両方とも

while (!frontiers.empty()) {

になり,それぞれのループの目的の違いがわかりにくいかなと思って外側に出しましたが,むしろ逆にわかりにくくなっていたようです.

元の私の意図はコメントを1行入れれば達成されるので,それをした上で変数のスコープをできるだけ短く保つことを優先した方が良いと思いました.

class Solution {
public:
vector<vector<int>> zigzagLevelOrder(TreeNode* root) {
vector<vector<int>> result;
Copy link

Choose a reason for hiding this comment

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

result という変数名は、設問に対する答え、または関数の返り値という意味しかないように思います。中にどのような要素が含まれているかを、端的な英単語・英語句で表現したほうが、読み手に取って有益だと思います。自分なら nodes_in_zigzag_order などとすると思います。

TreeNode* node = frontiers.back();
frontiers.pop_back();

if (traverse_from_left) {
Copy link

Choose a reason for hiding this comment

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

以下のように書くこともできるのですが、シンプルになっているかどうかは微妙に感じました。

TreeNode* children[] = {node->left, node->right};
if (!traverse_from_left) {
    std::swap(children[0], children[1]);
}
for (TreeNode* child : children) {
    if (child) {
        next_frontiers.push_back(child);
    }
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます.「例外なら,例外ケースを標準ケースに帰着させる」パターンとしてswapによる入れ替えも選択肢に入ることに気づけました.

一つ質問なのですが,

std::array<TreeNode*, 2> children{node->left, node->right};

のようにarrayでなくC配列を用いた特別な意図ってあったりしますでしょうか?

Copy link

Choose a reason for hiding this comment

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

特にありません。 std::array を使ったほうが C++ っぽく見えると思います。
ただ、やりたいことに対してやや見た目が複雑だなぁという気持ちもあります。

}

private:
vector<int> GetVectorFromDeque(std::deque<TreeNode*> nodes_deque) {
Copy link

Choose a reason for hiding this comment

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

Get という英単語は、既に存在している何らかのインスタンスを取得することを表すように思います。今回は deque を vector に変換しているため、 Get という単語はふさわしくないように思います。また、入力が deque であることは引数の方から自明のため、関数名に含める必要はないと思います。 ConvertToVector() または単に ToVector() はいかがでしょうか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Get という英単語は、既に存在している何らかのインスタンスを取得することを表すように思います。

この感覚がなかったので,言語化していただいて非常にありがたいです.また

入力が deque であることは引数の方から自明のため、関数名に含める必要はないと思います。

についても,確かにおっしゃる通りと納得しました.
ConvertToVector(), ToVector()はわかりやすくて良いと思います!


private:
vector<int> GetVectorFromDeque(std::deque<TreeNode*> nodes_deque) {
vector<int> nodes_vector;
Copy link

Choose a reason for hiding this comment

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

return vector<int>(nodes_deque.begin(), nodes_deque.end());

のほうがシンプルだと思います。

}

private:
vector<int> GetVectorFromDeque(std::deque<TreeNode*> nodes_deque) {
Copy link

Choose a reason for hiding this comment

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

引数が値型になっており、 deque 全体のコピーが作られ、無駄な処理が行われているように感じました。 CPU のレジスター幅 64-bit より十分大きく、関数内で変更しないインスタンスについては、 コピーが走るのを避けるため、 const 参照で渡すことをおすすめします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ミスですね...
変数の型への意識が薄いように思うので,気をつけて行きたいです.

while (!next_frontiers.empty()) {
frontiers = std::move(next_frontiers);
next_frontiers.clear();
result.push_back(GetVectorFromDeque(frontiers));
Copy link

Choose a reason for hiding this comment

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

emplace() を使って、中に入る要素のコンストラクターの引数を指定して要素を追加することができることを利用し、以下のように書くこともできます。

result.emplace(frontiers.begin(), frontiers.end());

ただ、やや分かりにくいかもしれません。

Copy link
Owner Author

Choose a reason for hiding this comment

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

この視点はなかったです.GetVectorFromDeque()のところでもコメントをいただきましたが,各コンテナのコンストラクタについては良く確認しておきたいです.

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.

3 participants