Skip to content

Create 543. Diameter_of_Binary_Tree.md#17

Open
Kitaken0107 wants to merge 2 commits intomainfrom
Kitaken0107-patch-19
Open

Create 543. Diameter_of_Binary_Tree.md#17
Kitaken0107 wants to merge 2 commits intomainfrom
Kitaken0107-patch-19

Conversation

@Kitaken0107
Copy link
Owner

left = self.depth(node.left) if node.left else 0
right = self.depth(node.right) if node.right else 0

self.diameter = max(self.diameter,left + right)
Copy link

Choose a reason for hiding this comment

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

, のあとに 1 つスペースを空けましょう。
https://peps.python.org/pep-0008/#whitespace-in-expressions-and-statements

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。
,のあとに詰めて書いてしまったコードが多いので、すべて修正させていただきます。
承知しました。
該当箇所のpep8読ませていただきました。

self.diameter = max(self.diameter,left + right)
return max(left,right) + 1

def diameterOfBinaryTree(self,root):
Copy link

Choose a reason for hiding this comment

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

diameterOfBinaryTree() を異なる木に対して複数回呼んだ場合に、結果が意図しないものになってしまう点が気になりました。複数の木に対して読んでも正しい結果となるような実装にするとよいと思います。
https://ja.wikipedia.org/wiki/%E5%8F%82%E7%85%A7%E9%80%8F%E9%81%8E%E6%80%A7

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.

参照透過性知らなかったです。ありがとうございます。
誤操作・誤作動のもとになりそうなので注意しますが、
修正方法ぱっと思い浮かばないので、
後ほど時間かけて取り組ませていただきます。
(再度、修正版のちほどあげさせていただきます。)

Copy link

Choose a reason for hiding this comment

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

修正方法は、

  1. 返り値をペアにするか
  2. 他の言語だと引数に参照を渡すとかができるのですが、Python だとそれは難しいのでリストを渡してそのリストの0番目をみんなで書き換えるか
    あたりです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

方向性教えていただきありがとうございます。
特に1で返り値をペアで出力できることをそもそも知らなかったです。
こちら後日取り組ませていただきます。

Copy link

Choose a reason for hiding this comment

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

複数回呼んだ場合に結果が意図しない、のは、絶対に駄目というわけではないのです。

たとえば、HTML の Parser みたいなものだと、複数回呼べなくても自然かなとは思います。抽出した情報を返す仕事を別のクラスにしないというのも選択だからです。

ただ、これくらい単純なものだと、しない選択肢もあるだろうに、何を代償にわざわざそうしたんだろう、という感覚です。

def __init__(self):
self.diameter = 0

def depth(self,node):
Copy link

Choose a reason for hiding this comment

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

関数名は、関数がどのようなことを行うかを端的に表す英単語または英語句とし、動詞で始めることをお勧めいたします。 get_depth() としたいですが、 self.diameter の更新も行っているため、不正確に感じます。 traverse() あたりがよいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

traverse()、承知いたしました。
ありがとうございます。

def __init__(self):
self.diameter = 0

def calc_depth_diameter(self,node):
Copy link

Choose a reason for hiding this comment

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

原則単語は省略せずにフルスペルで書くことをお勧めいたします。 number of を表す num_ や、 max number of を表す max_ 等、広く用いられているものについては使っても良いと思います。
https://google.github.io/styleguide/pyguide.html#316-naming

また、計算はしているとは言えないため、 calculate で始めるのは違和感があります。

Copy link
Owner Author

Choose a reason for hiding this comment

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

原則省略せずにフルスペル承知いたしました。
社内のかき捨てのコードに影響されてしまっていると思うので、意識して直します。
ほかの命名だとupdate_depth_diameter(ただ、depthが更新でよいのか気になります)などが思いつきますが、
以前教えていただいたtravarse単体で対応しようと思います。

Copy link

Choose a reason for hiding this comment

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

別問題別言語ですがこれが参考になるでしょう。
https://discord.com/channels/1084280443945353267/1206101582861697046/1230424716645240892

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