Vtyjkyuk

I was trying to solve this problem from Leetcode:

Given a string, find the length of the longest substring without
repeating characters.

Examples:

Given "abcabcbb", the answer is "abc", which the length is 3.

Given "bbbbb", the answer is "b", with the length of 1.

Given "pwwkew", the answer is "wke", with the length of 3. Note that
the answer must be a substring, "pwke" is a subsequence and not a
substring.

I would like to get some code review for the following solution:

class Solution:
 def lengthOfLongestSubstring(self, s):
 dict = 
 start, end, curr_length, max_length = 0, 0, 0, 0
 while ( end < len(s)):
 char = s[end]

 if char not in dict:
 curr_length += 1
 else: 
 start = max(dict[char], start)

 max_length = max(max_length, end - start + 1)
 dict[char] = end + 1
 end += 1
 return max_length

"""
 my logic is that
 * 1) Initialize a hashmap/dictionary/object that will map characters to their index.
 * 2) Initialize `start`, `end`, and `answer`
 * 3) Loop through the string while `end` is less than `length` of string
 * 4) Check if character at `end` is in hashmap
 * a) If so, repeating character exists between `start` and `end`. Set `start` to be the `max_length` between the value of character in the hashmap, and `start`

"""

And it passes all of the leetcode tests.

Nice solution, please find a few thoughts below:

1. Do you need to wrap it in class for leetcode? It's unnecessary otherwise.




2. Function names use snake_case and not camelCase, check PEP8 for more details. Hence it would be length_of_longest_substring.




3. Do not use dict as a variable name, it is already built-in to construct dictionaries.




4. I think a for-loop would be preferable over a while-loop in this case, it's more straightforward, less clunky, and easier to understand.




5. You update curr_length, but never actually use it to determine your max_length.




6. The docstring should come right after the class Solution and before the first function, and no line should be longer than 79 characters. Moreover, you seem to not have pasted the entire docstrings, it just ends mid-sentence.

Minor technical issues

There are a couple of minor technical issues with the solution:

• When looping over an iterable, if you need both the index and the value of the current item, it's good to use enumerate, so that you cannot accidentally forget to increment the loop counter, and syntax is nicely compact yet readable.




• curr_length is modified but never used: should be removed.




• The code is most readable when there is one statement per line. So I recommend to split multi-assignments like start, end, curr_length, max_length = 0, 0, 0, 0. Simple multi-assignments with 2 terms are usually ok though, for example left, right = ... when implementing a binary search




• The parentheses are unnecessary around the condition in while end < len(s):




• It's unfortunate use names that are types in many languages, such as dict and char




• PEP8 recommends to indent with 4 spaces. And it strongly recommends to use the same indent width consistently. (The posted code indents the class body by 4 spaces, but everything else with 2.)

Alternative implementation

Putting the above tips together, and throwing in some doctests:

class Solution(object):
 def lengthOfLongestSubstring(self, s):
 """
 >>> Solution().lengthOfLongestSubstring("bpfbhmipx")
 7

 >>> Solution().lengthOfLongestSubstring("abcabcbb")
 3

 >>> Solution().lengthOfLongestSubstring("bbbbb")
 1

 >>> Solution().lengthOfLongestSubstring("pwwkew")
 3

 >>> Solution().lengthOfLongestSubstring("tmmzuxt")
 5

 >>> Solution().lengthOfLongestSubstring("a")
 1

 >>> Solution().lengthOfLongestSubstring("")
 0

 >>> Solution().lengthOfLongestSubstring("abc")
 3

 """

 longest = 0
 start = -1
 last = 

 for i, c in enumerate(s):
 if c in last:
 start = max(start, last[c])
 longest = max(longest, i - start)
 last[c] = i

 return longest

just a note, for string iteration, a for loop might be more suitable

from

 while ( end < len(s)):
 char = s[end]
 ...
 end += 1

to

for end, char in enumerate(s)
 ...