Skip to content

3. Longest Substring Without Repeating Characters #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fhiyo
Copy link
Owner

@fhiyo fhiyo commented Jul 20, 2024

Copy link

@hayashi-ay hayashi-ay left a comment

Choose a reason for hiding this comment

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

良いと思います。

def lengthOfLongestSubstring(self, s: str) -> int:
char_to_index = {}
max_length = 0
left = -1
Copy link

Choose a reason for hiding this comment

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

個人的には 2nd のように left = 0 から始める方が好きですが、 left = -1 から始めても良いと思います。

setってremove(), clear()だけじゃなく、discard()やpop()もあるのか。[Built-in Types — Python 3.12.4 documentation](https://docs.python.org/3.12/library/stdtypes.html#set)
discard(elem)はelemが無くてもエラーにならない、pop()は要素があるならelemを返す。

辞書にすると、管理にかかる計算コストが少し少なくなるのか。setにする方が素直だとは思うが。
Copy link

@thonda28 thonda28 Jul 20, 2024

Choose a reason for hiding this comment

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

個人的には dict の方が while 文が不要で理解しやすく感じました。left をずらしながら set から要素を削除する処理が不要な分、(書かれている通り)計算コストも少なそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど...setの解法の場合、setの中身が注目している部分文字列の文字集合だという点が分かりやすいので好みなのですが、人それぞれなのかもですね

Choose a reason for hiding this comment

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

setの解法の場合、setの中身が注目している部分文字列の文字集合だという点が分かりやすいので好みなのですが、人それぞれなのかもですね

そうですね、人それぞれなのかもです。個人的には、各文字の index 情報があるのに利用しないのがもったいないという感覚がありました。

max_length = 0
left = 0
char_to_index = {}
for right in range(len(s)):

Choose a reason for hiding this comment

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

合計4回 s[right] を使っているのが冗長かつ Typo のようなミスが起きやすいのでは、と感じました。Python の場合は enumerate を使うと s[right] と繰り返し書く必要がなく、可読性が高まりそうです。また enumerate を使わない場合も、わかりやすい命名の一時変数で s[right] を扱ってもよさそうです。

Suggested change
for right in range(len(s)):
for right, char in enumerate(s):

Copy link
Owner Author

Choose a reason for hiding this comment

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

enumerateや一次変数に入れるより、s[right]の方が文字列のright番目を取り出す意図がはっきりして分かりやすいかなと思って書いたんですが、うーむ好みの違いなのかもですね。
typoの起きやすさや冗長さ、 s[i] のように書くと微妙になりそうですかね?そこの感覚はあまり無かったです。

Choose a reason for hiding this comment

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

たしかに元のコードのほうが可読性が高いのかもしれません。(僕もこの問題を解いたことがあったので)読み手というより書き手側の視点で見てしまっていたかもです。とくにもっと処理が大きくなったときには char のようなものよりは s[right] の方が読みやすそう(理解しやすそう)ですね。

コードを書く側の目線で見たときに enumerate が楽だなと思ってのコメントでしたが、重要なのは読み手の視点なので上のコメントは適切でなかったかもです。

if end == len(s):
break
chars.remove(s[begin])
return max_length

Choose a reason for hiding this comment

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

ここはbegin, endに変更した理由が特になければ、step1, 2のようにleft, rightの方が読みやすく感じました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ループ不変条件が [begin, end) の区間に注目してるので半開区間であることを主張したく、begin-endを使ってみました。
right使ったときは右側はinclusiveだよということを主張したつもりでした

@Mike0121
Copy link

見ました。コメントも含め、色々な書き方・解き方が検討されており良いと思いました。

- https://discord.com/channels/1084280443945353267/1196472827457589338/1246368719148548117
- https://github.com/Mike0121/LeetCode/pull/21

setってremove(), clear()だけじゃなく、discard()やpop()もあるのか。[Built-in Types — Python 3.12.4 documentation](https://docs.python.org/3.12/library/stdtypes.html#set)
Copy link

Choose a reason for hiding this comment

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

ドキュメント読むの大事ですね。

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.

6 participants