Conversation
| // 時間計算量: O(n) | ||
| // 空間計算量: O(n) | ||
| class Solution { | ||
| public int minDepth(TreeNode root) { |
There was a problem hiding this comment.
はい、次のように出来ます
class Solution {
public int minDepth(TreeNode root) {
if (root == null) {
return 0;
}
if (root.left == null && root.right == null) {
return 1;
}
int leftNodeMinDepth = Integer.MAX_VALUE;
if (root.left != null) {
leftNodeMinDepth = minDepth(root.left);
}
int rightNodeMinDepth = Integer.MAX_VALUE;
if (root.right != null) {
rightNodeMinDepth = minDepth(root.right);
}
return Math.min(leftNodeMinDepth, rightNodeMinDepth) + 1;
}
}| minDepth++; | ||
| } | ||
|
|
||
| return MIN_DEPTH_NOT_CALCULATED; |
There was a problem hiding this comment.
Depth の値として期待されない -1 を返す代わりに Exception を throw することも考えたが、後続処理にどのような影響を与えるべきかの議論なしにそこまでやるのは憚られたのでこのようにした
期待されない値というか、ここって到達しないので、throw Exception("unreachable")が正しいんじゃないでしょうか??
There was a problem hiding this comment.
自分もここは気になりました。アルゴリズム上到達しない箇所にreturnが書いてあったらどこかで実装ミスをしたのかな、と思って混乱するかもなと思いました
There was a problem hiding this comment.
到達しないつもりで書いてはいるのですが、コンパイラによってチェックされているといった保証が無いので、到達する可能性は排除出来ない認識でいます。
仮にユーザーがこの関数を含めた複数の処理を手続き的に実行する場合、予期せずこの関数が Exception を throw してしまうと後続処理が止まってしまいます。例えば呼び出し元が次のようになっている場合、この関数で Exception が発生したら logics.somethingVeryImportantMustBeReached(valCanBeTrivial) は実行されません。
public class Driver {
public static void main(String[] args) {
TreeNode root = /* 木の構築 */;
Solution solution = new Solution();
OtherLogics logics = new OtherLogics();
try {
int minDepth = solution.minDepth(root);
int valCanBeTrivial = logics.somethingTrivial(minDepth);
logics.somethingVeryImportantMustBeReached(valCanBeTrivial);
} catch (Exception e) {
System.err.println("エラーが発生しました: " + e.getMessage());
}
}
}特に確認が出来ていない状況であれば、とりあえず選ぶならば、後続処理を止める可能性のある実装より、そうでない実装の方にした方がまだいいのかなと考えていました。
There was a problem hiding this comment.
とは言うものの、そもそもチェック例外のある関数を呼ぶなら呼ぶ側が気をつければいいという話ではあるかもしれないですね... と上記書いてて思いました。
上記の後続処理も、絶対実行させたいなら次のように書くことも出来ますし。
public class Driver {
public static void main(String[] args) {
TreeNode root = /* 木の構築 */;
Solution solution = new Solution();
OtherLogics logics = new OtherLogics();
int valCanBeTrivial = 0; // デフォルト値
try {
int minDepth = solution.minDepth(root);
valCanBeTrivial = logics.somethingTrivial(minDepth);
} catch (Exception e) {
System.err.println("エラーが発生しました: " + e.getMessage());
} finally { // 👈 例外の発生有無に関わらずこのブロック内の処理は必ず実行される
logics.somethingVeryImportantMustBeReached(valCanBeTrivial);
}
}
}There was a problem hiding this comment.
私がよく分かっていない可能性があるのですが、
到達しないつもりで書いてはいるのですが、コンパイラによってチェックされているといった保証が無いので、到達する可能性は排除出来ない認識でいます。
とあり、どのような入力を与えると(若しくは事故?が起こると)return MIN_DEPTH_NOT_CALCULATED;に到達することがあるのか気になっております。
There was a problem hiding this comment.
特に確認が出来ていない状況であれば、とりあえず選ぶならば、後続処理を止める可能性のある実装より、そうでない実装の方にした方がまだいいのかなと考えていました。
なるほどありがとうございます。まあ例外を避けたいという気持ちも分かりますし、returnにするのもあるかもなという気持ちになりました
There was a problem hiding this comment.
私がよく分かっていない可能性があるのですが、
到達しないつもりで書いてはいるのですが、コンパイラによってチェックされているといった保証が無いので、到達する可能性は排除出来ない認識でいます。
とあり、どのような入力を与えると(若しくは事故?が起こると)
return MIN_DEPTH_NOT_CALCULATED;に到達することがあるのか気になっております。
すいません、返信がかなり遅くなってしまいました。
私が認識してる限りですと、この関数に関してはこの行に到達するケースは無いです。ただそれはあくまで人間 (今回の場合は実装者の私) がチェックした結果そう認識しているだけで、型システムなどで保証されているわけではありませんので、到達する可能性は排除できないと書いていました。
There was a problem hiding this comment.
これだけ多くの人が見ても見つからないのでこの関数に関してはもうこの行に到達することはないと考えていいと思います。ただそれはそれとして、システムによる保証がない状況で強いて選ぶなら... と考えてこのようにした次第です。
There was a problem hiding this comment.
あ、これなんですが、(別のスレッドからこの変数をどうにかしていじるなどがなければ)到達しないと思うのですが「コンパイラが到達しないことを理解してくれるか」は別問題です。一般に、一重ループの停止性問題は決定不能なので、コンパイラが停止するかしないかを100%の精度で当てることはありません。
あと、コードを読んでいる人にも、停止性問題を解かせないで欲しいのでコメントを残すなりしましょうか。
私は無限ループへの書き換えを推します。
| // 時間計算量: O(n) | ||
| // 空間計算量: O(n) | ||
| class Solution { | ||
| private static final int MIN_DEPTH_NOT_CALCULATED = -1; |
There was a problem hiding this comment.
calculate には、計算式を用いて何らかの計算をするという意味合いがあるように思います。このプログラムは特に計算をするわけではないため、 calculate という単語を使うのは不適切なように思います。別の単語を使うことをお勧めいたします。
There was a problem hiding this comment.
@nodchip
ありがとうございます。確かにそうですね。
MIN_DEPTH_NOT_FOUND が適切なのかなと考えましたが、こちらはいかがでしょうか?
There was a problem hiding this comment.
ありがとうございます!こちら Step 4 の方に書いておきます。
| while (!treeNodeDepths.isEmpty()) { | ||
| TreeNodeDepth nodeDepth = treeNodeDepths.pop(); | ||
| if (nodeDepth.node.left == null && nodeDepth.node.right == null) { | ||
| System.out.println(nodeDepth.node.val); |
There was a problem hiding this comment.
レビューワーにとってノイズになる可能性があるため、デバッグ出力は削除してからコードレビューを依頼することをお勧めいたします。
There was a problem hiding this comment.
大変失礼しました。取り急ぎ削除致しました。
| return 1; | ||
| } | ||
|
|
||
| int leftNodeMinDepth = Integer.MAX_VALUE; |
There was a problem hiding this comment.
int minDepth = Integer.MAX_VALUE;
if (node.left != null) {
minDepth = Math.min(minDepth , traverseMinDepthRecursively(node.left));
}
if (node.right != null) {
minDepth = Math.min(minDepth , traverseMinDepthRecursively(node.right));;
}
return minDepth + 1;
とすると、少しシンプルになると思います。
There was a problem hiding this comment.
ありがとうございます!Step 4 に書いておきます。
| return traverseMinDepthRecursively(root); | ||
| } | ||
|
|
||
| private int traverseMinDepthRecursively(TreeNode node) { |
There was a problem hiding this comment.
traverse は他動詞としては
https://eow.alc.co.jp/search?q=traverse
〔山などを〕越える
〔場所を〕横切る、横断する
〔~に〕矛盾する、反対する、抗弁する、否認する
〔~を〕詳しく検討する
という意味があり、目的語を min depth にすると意味が通らなくなってしまうように思います。別の表現は思いつきますか?
There was a problem hiding this comment.
@nodchip
なるほどありがとうございます。木に対する走査を tree traversal と言うのでその動詞形を選んでいましたが、引用していただいた他の意味を考えると適切でないように思えました。あと tree を traverse することは手段であって目的ではないのでやはり不適切ですね。
別の表現は思いつきますか?
findMinDepthRecursively を思いつきました。問題文に "Given a binary tree, find its minimum depth" とあるので、自然な命名になっていると感じますがいかがでしょうか。
| minDepth++; | ||
| } | ||
|
|
||
| return MIN_DEPTH_NOT_CALCULATED; |
There was a problem hiding this comment.
自分もここは気になりました。アルゴリズム上到達しない箇所にreturnが書いてあったらどこかで実装ミスをしたのかな、と思って混乱するかもなと思いました
| - [ArrayDeque はドキュメント冒頭にある通り早い](https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/java.base/java/util/ArrayDeque.html)ので、その利点を捨ててまで、例えば null 許容する LinkedList を implementing class にすることもないかなと考える | ||
| - minDepth() の最下部は次のようなことを考えて実装した: | ||
| - 処理に異常がない限りは到達しないことを明記 | ||
| - Depth の値として期待されない -1 を返す代わりに Exception を throw することも考えたが、後続処理にどのような影響を与えるべきかの議論なしにそこまでやるのは憚られたのでこのようにした |
There was a problem hiding this comment.
自分としてはException を throwすることは憚られるが-1を返すのは良いという理屈があまり分かりませんでした。
There was a problem hiding this comment.
別のスレッドに返信していたので一応こちらにもリンク貼っておきます: #21 (comment)
| if (nodeDepth.node.left != null) { | ||
| treeNodeDepths.push(new TreeNodeDepth(nodeDepth.node.left, nodeDepth.depth + 1)); | ||
| } | ||
| if (nodeDepth.node.right != null) { | ||
| treeNodeDepths.push(new TreeNodeDepth(nodeDepth.node.right, nodeDepth.depth + 1)); | ||
| } |
There was a problem hiding this comment.
細かいですが、stack-dfsは右>左の順に書いたほうが自然な処理の流れで好みです。
| minDepth++; | ||
| } | ||
|
|
||
| return MIN_DEPTH_NOT_CALCULATED; |
| } | ||
|
|
||
| // 処理に異常が無い限りここには到達しません。 | ||
| return MIN_DEPTH_NOT_CALCULATED; |
There was a problem hiding this comment.
!treeNodes.isEmpty になることがないはずなので、無限ループにしてこの行消したほうがいいのかなと思いました。
https://leetcode.com/problems/minimum-depth-of-binary-tree/description/