-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor #1
base: main
Are you sure you want to change the base?
refactor #1
Conversation
@@ -1,66 +1,50 @@ | |||
class Solution: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a class here
@@ -1,66 +1,50 @@ | |||
class Solution: | |||
# Problem: | |||
# Given an array of integers and target integer, return the indexes of two array elements that sum to the target, or None if doesn't exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a problem statement.
|
||
def brute_force(nums: [int], target: int) -> [int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of commenting out the code, you can use separate functions
|
||
def twoSum(self, nums: List[int], target: int) -> List[int]: | ||
|
||
#test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just commenting the test cases, write code to execute them
#[1,2] target = 3 | ||
#[1,-2] target = -1 | ||
#[1,3,2] target = 5 | ||
#[0,1,4,8] target = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A key test case is what happens if the sum doesn't exists
#the difference of the current integer and target already. | ||
#we stop searching when we found a pair of numbers that equal the target or when we checked the entire array with no match. | ||
#checking the input array is O(n) and for every integer we check, say a dictionary, which would be O(1) on average | ||
#but can possibly take O(n) worst case so it is between O(n) and O(n^2) ( O(nlogn) ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this is O(n^2), but you don't have to go into details, and for the hash map you can just describe the expected run time
|
||
|
||
def twoSum(self, nums: List[int], target: int) -> List[int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a different version of python, but List in the type specifier wasn't found for me
|
||
if target- nums[i] in checkedInts: | ||
|
||
return i,checkedInts[target- nums[i]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that you specified the return type as [int], but this I think returns a tuple - in my solution I changed it to return a list, but tuple would be fine as well.
("aaaaab", "b"), | ||
("bbbbba", "bbbbb"), | ||
("abababa", "bbb") | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend being explicit about the expected value of the tests. Some other tests to think of adding would be to cover all letters of the alphabet and make sure they have the appropriate behavior. Also would be good to be explicit about upper case and non-alpha (having test cases is a good way to document expected behavior).
|
||
return False | ||
def isMatch(c1, c2)->bool: | ||
return (c1, c2) in valid_pairs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This saves a lot of code
No description provided.