Skip to content

Conversation

@Satorien
Copy link
Owner

def lengthOfLongestSubstring(self, s: str) -> int:
substring_start_position = -2
max_length = 0
char_to_position = defaultdict(lambda: -1)

Choose a reason for hiding this comment

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

デフォルトにラムダを渡すのは少しトリッキーに感じました(自分がPythonに浅いためかもですが)
50行を、duplicate_position = char_to_position.get(char, -1)にすればchar_to_positionは通常の辞書で定義することができます。

この方法はEffective Python第2版の16行目で紹介されております。最近3版が出たのでそこで消えていたらすみません。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。その方が読みやすいですね

```python
class Solution:
def lengthOfLongestSubstring(self, s: str) -> int:
substring_start_position = -2

Choose a reason for hiding this comment

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

このマジックナンバーは下のコードを読まないと何をしているのかわからないので説明を入れるか、50行目をgetの方法にして-1で初期化する方がわかりやすいのかなと思いました。

chars_in_window = set()
left_index = 0
max_length = 0
for right_index, char in enumerate(s):

Choose a reason for hiding this comment

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

charはC++だと予約語なので自分なら使わないなと思いました。
好みやチームの方針によるのかなと思います。

max_length = 0
char_to_position = defaultdict(lambda: -1)
for index, char in enumerate(s):
duplicate_position = char_to_position[char]
Copy link

Choose a reason for hiding this comment

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

duplicate_position ですと、 duplicate が動詞のため、関数名のように感じられました。また、直訳すると「重複した位置」となり、表現したいこことずれているように感じされました。自分なら、 last_position と名付けると思います。

substring_start_position = -2
max_length = 0
char_to_position = defaultdict(lambda: -1)
for index, char in enumerate(s):
Copy link

Choose a reason for hiding this comment

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

position と index が同じ意味で使われている点に違和感を感じました。どちらかに統一するとよいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

どちらが良いか迷って混在させてしまってました。修正するようにします

Comment on lines +54 to +55
max_length = max(max_length, \
index - substring_start_position + 1)

Choose a reason for hiding this comment

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

Suggested change
max_length = max(max_length, \
index - substring_start_position + 1)
max_length = max(
max_length,
index - substring_start_position + 1,
)

こちらのスタイルの方がよく見る気がします。
ちなみにですがpythonではカッコ(()、{}、[])の中では自由に改行できます。

def lengthOfLongestSubstring(self, s: str) -> int:
max_length = 0
substring = ""
for char in s:

Choose a reason for hiding this comment

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

charはC、C++、Javaなどの予約語(基本データ型)charと被るので他言語に慣れた読み手に配慮して自分は避けるようにしています。

cかchがよくみます。ちなみにchrはPythonの組み込み関数にあるので避けます。

Comment on lines +89 to +91
substr_start = 0
chars_in_substr = set()
for substr_end, char in enumerate(s):

Choose a reason for hiding this comment

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

start, endはsliding windowを閉区間で表した時の両端点として使われていますが、Python documentationなどでは一貫して半開区間[start, end)の両端点として使われています。

例:https://docs.python.org/3/library/stdtypes.html#str.find

str.find(sub[, start[, end]])

Return the lowest index in the string where substring sub is found within the slice s[start:end]. Optional arguments start and end are interpreted as in slice notation. Return -1 if sub is not found. For example: ...

ちなみに、start, stopも同様です。
なので自分は閉区間の両端点を誤解なく表現したいときはfirst, lastやhead, tail、もしくは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.

確かにその観点は抜けていたので参考にします

Comment on lines +103 to +112
class Solution:
def lengthOfLongestSubstring(self, s: str) -> int:
max_length = 0
substring_start = 0
for substring_end, char in enumerate(s):
duplicate_position = s.find(char, substring_start, substring_end)
if duplicate_position != -1:
substring_start = duplicate_position + 1
max_length = max(max_length, substring_end - substring_start + 1)
return max_length

Choose a reason for hiding this comment

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

substring_startの左隣を変数に置くと更新式で+1の修正が不要になり、またsubstringの距離も半開区間の両端点の差になってわかりやすくなるかもです。

class Solution:
    def lengthOfLongestSubstring(self, s: str) -> int:
        max_length = 0
        passed_tail = -1
        for substring_tail, ch in enumerate(s):
            last_found = s.find(ch, passed_tail + 1, substring_tail)
            if last_found != -1:
                passed_tail = last_found
            max_length = max(max_length, substring_tail - passed_tail)
        return max_length

Choose a reason for hiding this comment

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

上の

if last_found != -1:
    passed_tail = last_found

のところは、

passed_tail = max(passed_tail, last_found)

とも書けますね。これは趣味の範囲だと思います。

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.

5 participants