111. Minimum Depth of Binary Tree

[usatie]

問題 : https://leetcode.com/problems/minimum-depth-of-binary-tree
言語 : C++
step3をやる際に、念の為ローカルでテストができるようにmain.cppとMakefileも用意しています。
テスト用に書いたstd::initializer_listからbuildTreeする部分自体もleetcodeチックで、もっといい書き方があるはず。

[oda]

正直、テストコードのテストコードが欲しくなりますね。
テストはできるだけ、繰り返しが多くても単純にしたいので、下のようなのを作りますかね。

TreeNode* node =
    new TreeNode(
        0,
        new TreeNode(
            1,
            new TreeNode(
                3
            ),
            new TreeNode(
                4
            )
        ),
        new TreeNode(
            2
        )
    );

[usatie]

正直、テストコードのテストコードが欲しくなりますね。

そうですよね。

テストケースの数だけ、こういう風に書くイメージですか？

  test(nullptr, 0);
  test(new TreeNode(1), 1);
  test(new TreeNode(3,
                    new TreeNode(9),
                    new TreeNode(20,
                                 new TreeNode(15),
                                 new TreeNode(7))),
       2);
  test({2, null, 3, null, 4, null, 5, null, 6}, 5);
  test(new TreeNode(2, nullptr,
                    new TreeNode(3, nullptr,
                                 new TreeNode(4, nullptr,
                                              new TreeNode(5, nullptr,
                                                           new TreeNode(6))))),
       5);

[oda]

実際は、テストフレームワークをなんらか使うはずで、そうなると大体の場合は、テストケース一つにつき関数を一つ作ることになるでしょう。また、再帰的な delete は別にすることになるでしょう。Address Sanitizer のような終了時のメモリー状態を確認する機能が動作するようにするためです。
https://google.github.io/googletest/gmock_cook_book.html

[oda]

ヘッダーの順番は、標準ライブラリーが上のことが多いと思いましたが、これは、step3.hpp をテストするから一番上なんですかね。

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[usatie]

今回はclang-formatを使用しなかったためでした。
この辺のヘッダーの並び替えは普段は大体clang-formatに任せているので以前に調べたっきり、ぼんやりとしか意識していませんでした。

ところで、改めて読んでみると Relative headerを最初に載せるということが書いてあってそれは知りませんでした。今回の場合ですと、名前はmain.cppになっているのであまり良くない気がしますが、もしこれがtest_step3.cppとかであれば、この順番でもいいんでしょうか。

Include headers in the following order: Related header, C system headers, C++ standard library headers, other libraries' headers, your project's headers.

[oda]

これは struct TreeNode の上を勧めます。

[oda]

これは、これでいいですが、大規模開発の場合は分割コンパイルをします。
その時には、ヘッダーファイルには
int minDepth(TreeNode *root);
と宣言だけして、
.cpp に
int Solution::minDepth(TreeNode *root) {
}
本体を書きます。

[oda]

while (1) にしてしまうのは一つだと思います。

[t0hsumi]

dfsという命名に関しては、discordで議論がいくつかあるので見てみてください

[t0hsumi]

Queueであることは、型名から自明なのでなくて良いと思います。
TreeNodeとintが何を表しているか明確にするために、nodes_and_depthsとかを使っているコードをよくみます。

個人的には、小さい関数で関数名から何をするか大体わかるし、c++なら型を明記できるので、next_visitとか中身を明記しない変数名でもいいとは思います。

[usatie]

ご指摘いただいた点、確かに私も同感です。

nodes_and_depthsという名称は、変数に格納される要素（ノードと深さのペア）の型情報は明示しているものの、アルゴリズム上の役割、つまり「これから訪問するべきノード群」という意図を十分に伝えられていないと感じました。一方、next_visitでは単一の要素を連想させてしまうため、どちらも意図を正確に表現できていないと考えています。

そのため、たとえばnodesToVisitやpendingVisits、またはBFSの文脈でよく用いられるfrontierなど、複数形や集合を示唆する名称の方が意図を正確に伝えられるかと思います。

今後の実装では、こうした名称を検討してみたいと思います。

[usatie]

また、やや今回のケースではやりすぎな感もありますが、このように型情報として中に含まれているのがdepthであることを明示するのも一つの手かもしれません。

typedef int depth_t;
std::queue<std::tuple<TreeNode*, depth_t>> nodesToVisit;

[nodchip]

個人的にはローカルの変数名や引数は lower_snake にすることが多いですが、所属するチームの平均的な書き方に合わせれば問題ないと思います。

[nodchip]

numInThisDepth 個だけキューの要素を処理するという書き方は、個人的にはややもやっとします。 2 つの vector を用意し、交互に使用する感じのほうが、個人的にはすっきりしていると思います。

std::vector<TreeNode *> nodes;
int depth = 1;
nodes.push_back(root);
while (!nodes.empty()) {
  std::vector<TreeNode *> nodesInNextDepth;
  for (auto node : nodes) {
    if (node->left == nullptr && node->right == nullptr) {
      return depth;
    }
    if (node->left) {
      nodesInNextDepth.push_back(node->left);
    }
    if (node->right) {
      nodesInNextDepth.push_back(node->right);
    }
  }
  std::swap(nodes, nodesInNextDepth);
  ++depth;
}

個人の好みの問題かもしれません。

[usatie]

ありがとうございます。

[colorbox]

{}のないifブロックはちょっとした編集ミスでif条件の位置がズレてバグるので、可能であれば{}で囲うことをおすすめします。
https://ttsuki.github.io/styleguide/cppguide.ja.html#Formatting_Looping_Branching

[nittoco]

left, rightは、left_depth, right_depthと、深さを表すことを示した方がわかりやすそうです。
TreeNode構造体の要素の名称と重なるのはやや抵抗感があります