-
Notifications
You must be signed in to change notification settings - Fork 47
Ports - Kasey #44
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
base: master
Are you sure you want to change the base?
Ports - Kasey #44
Conversation
|
This looks good to me! I like where you checked if both input arrays were nil or empty arrays...I didn't think of that. |
ace-n
left a comment
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.
👍 LGTM other than a few style nits. 😄
| # and the same integer values in the same exact order | ||
| def array_equals(array1, array2) | ||
| raise NotImplementedError | ||
| if array1 == nil && array2 == nil || array1 == [] && array2 == [] |
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.
Good boundary checking!
Style nit: this many if-statements all in one line is a bit confusing. (You have to know Ruby's order of operations in order to understand it. 😄 )
- Good: use parenthesis around the "and" clauses (e.g.
if (array1 == nil && array2 == nil) || (array1 == [] && array2 == [])) - Better: break up each condition (e.g.
== niland== []) into separate "guarding"if-statements
e.g.
# Null/empty checks
if array1 == nil && array2 == nil
return true
elsif array1 == [] && array2 == []
return true
elsif array1 == nil || array2 == nil
return false
# Length check
if array1.length != array2.length
return false
end
...
You might even be able to do this - but some people would say this
is not explicit enough (or "has too much magic")
if array1 == array2:
return True
elsif array1 == nil || array2 == nil:
return False
elsif array1 == [] || array2 == []:
return False
end
| elsif array1 == nil || array2 == nil || array1.length != array2.length | ||
| return false | ||
| else | ||
| i = 0 |
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.
Nit: this block shouldn't be nested inside an else (because the else is redundant here).
| return false | ||
| else | ||
| i = 0 | ||
| array1.each do |x| |
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.
Nit: .each iterates through arrays in order in Ruby, but may not for other data types
Would this break if (for whatever reason) .each did its iteration out of order? If so, how might we fix that?
No description provided.