617. Merge Two Binary Trees#22
Conversation
| } | ||
|
|
||
| for len(tasks) > 0 { | ||
| t := tasks[len(tasks)-1] |
There was a problem hiding this comment.
node1 = t.pair[0]、node2 = t.pair[1]と名前を付ける方が好みです。今のコードはnode1, node2とleft1, left2, right1, right2との対応が見づらく感じたからです。
また、tの使われる範囲が一文字変数にしては長めで抵抗があります。ただ個人的にはそれほどには読みづらくは感じなかったです。
There was a problem hiding this comment.
node1, node2に関して，その通りだなと思いました．ありがとうございます．
t については，taskだと，定義名と被っちゃうしなと思って，tasksからとりだしたんだからまぁわかるだろう，くらいの判断で一文字にしてました．
There was a problem hiding this comment.
（あ、Goは型名も小文字なんですね。それはたしかにちょっと命名に迷いますね。）
There was a problem hiding this comment.
https://go.dev/tour/basics/3
最初を大文字にすると、パッケージの外から見えるようになりますね。
| - https://github.com/hiroki-horiguchi-dev/leetcode/pull/23 | ||
| - 非破壊的なほうがいいという点には同意なんだよな．Goではどうしよう． | ||
| - https://github.com/jjysogfy/arai60-202603/pull/12 | ||
| - step1の20,21行目が改行入っている理由とかよくわからんかった．もう一度読んだらこれ関数呼び出しか．わかりづらくて好みではない． |
There was a problem hiding this comment.
読んでもらってありがとうございます。たしかに、その2行は一見すると独立した2文のようにも見えますね。
（Optionalをやめたほうが見やすいのはさておき）同じ方針で書き直すなら、var leftNode = mergeTrees(の直後でも改行するのがいいでしょうか。
自分のコードの話になってすみません。もしお時間あれば、お返事いただけるとうれしいです。
There was a problem hiding this comment.
コメントありがとうございます．
そうですね．これを読んだときに考えていたことは，以下のような感じです．
- 変数宣言で行頭を変数と同じ位置に揃えていたので，二つ変数を宣言したのかなとパッとみ思いました．
- で，よく読むと関数の引数の区切りで改行されていることに気づきました．
- 関数呼び出しの引数ならば，関数名よりもすこし下げた位置にインデントされているといいな，と感じました
個人の思考メモなので，ぶっきらぼうな書き方になってしまってすみません．
Javaのお作法についても全く踏まえずに書いてしまってますので，適当言ってる部分ありましたらすみません．
There was a problem hiding this comment.
変数宣言で行頭を変数と同じ位置に揃えていたので，二つ変数を宣言したのかなと
関数名よりもすこし下げた位置にインデントされているといいな，と
ありがとうございます。たしかに、と思いました。JavaのGoogleスタイルガイド的にも、スペース4つより深くインデントすることは許されていそうです。
個人の思考メモなので，ぶっきらぼうな書き方になってしまってすみません．
承知していますので、お気になさらず。むしろ読んでコメントいただけるのはありがたいです。
| // スタックオーバーフローになるかの簡単な検算をしておく | ||
| // N=2000 の配列が2つ受け取る．int32以上なら各Valは収まる． | ||
| // まぁでも，TreeNodeのstruct見る感じ，(24+8)くらいで32byteとして，これを二つ受け取る感じの関数が再帰することになりそう |
There was a problem hiding this comment.
どういう計算なのか気になりました。
コメント集やChatGPTを参考に、自分も見積もってみました。
https://discord.com/channels/1084280443945353267/1237649827240742942/1278333082201489491
見積もる前に、まず（64bit環境なら）ポインタは8Bで、TreeNodeの実体はヒープ領域にありスタック領域にはポインタだけがあると思いました。
- 引数：
2 * 8B = 16B - ローカル変数：とりあえずnextLeftのひとつだけと考えて、
8Bと見積もる - リターンアドレス：
8B - リターン値設定アドレス：
8Bと見積もる。ただしレジスタに収まるので、実際にはスタックには積まれなさそう - レジスタ退避域・一時変数：
0Bと見積もる - 合計で
40B- ただ、あくまでおおざっぱな見積もりなので、いろいろ増減があると思います。
自分はあまりちゃんと見積もりをやっていなかったので、考え方の違いなど、なにかあればコメントもらえるとうれしいです。
There was a problem hiding this comment.
すみません，これコメントが完全に古くなっているので，無視していただいて構いません．
ちょっと逸れますが，コメントいただいたことで以下の点について，今回は勘違いしていたことに気づきました．ありがとうございます．
TreeNodeの実体はヒープ領域にありスタック領域にはポインタだけがあると思いました。
元々考えていたことを思い出してみます，パッと考えてみたのですが，数日時間を置きながら解いたこともあり，支離滅裂なことになってしまっていそうです．（のちほど追記します）
There was a problem hiding this comment.
自分も、メモを適当にしてしまうことがよくあります。無理のない範囲で、何か思い出したら教えていただければ幸いです。
There was a problem hiding this comment.
たぶん以下です．
- （step1時点でコメントアウトしてますが）もともとmergeTreeFuncという再帰用の関数を用意して，それを利用するつもりでした．
- mergeTreeFuncの引数はもともと，root1,root2, resultTree（結果ノードを書き込む）の3つの引数がいるかなと検討をつけました．これらはいずれもポインタなので，よって8B * 3で24byte，あとはリターンアドレスが保持されるだろうと思い，+8Bして，32byteと計算した，気がしています．ですが最終的にstep1では
resultTreeは不要と判断しました． - ローカル変数の見積もりは完全に漏れていますね．これはこれまで毎回漏れていたように思います．勉強になりました．
まぁでも，TreeNodeのstruct見る感じ，(24+8)くらいで32byteとして，これを二つ受け取る感じの関数が再帰することになりそうという文章全体は，完全に意味不明だなと思います．これはもう無視してください．
| // スタックオーバーフローになるかの簡単な検算をしておく | ||
| // N=2000 の配列が2つ受け取る．int32以上なら各Valは収まる． | ||
| // まぁでも，TreeNodeのstruct見る感じ，(24+8)くらいで32byteとして，これを二つ受け取る感じの関数が再帰することになりそう | ||
| // 最大2000回のスタックなので，2*10^3 * 32 = 64 * 10^3 = 64KB程度 |
There was a problem hiding this comment.
最大2000回のスタックなので
日本語として不自然に感じました。「最大 2000 回(再帰関数が呼ばれて、スタックメモリに)スタックフレームが詰まれるので、」でしょうか。
There was a problem hiding this comment.
その通りですね．
別の方のコメントもあり，方針メモ（残すことにだけを重視しすぎて）を適当に書きすぎているなと思いました．
4 participants
https://leetcode.com/problems/merge-two-binary-trees/