-
Notifications
You must be signed in to change notification settings - Fork 47
Ports_Myriam #26
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_Myriam #26
Conversation
| return false | ||
| end | ||
| end | ||
|
|
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 job! That's a very elegant solution!
mmcknett
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.
Checklist
- Clean, working code
- Unfortunately, neither solution passes the edge cases defined in the spec file. Also, leftover test code is detracting from the code's cleanliness.
- Efficient code - give feedback as you would give to a peer on your team
- Specifically, solution 1 appears to be optimal, and returns as soon as it knows when the arrays are not equal.
A detailed explanation of time and space complexity (explains what n stands for, explains why it would be a specific complexity, etc.)[Not part of this assignment]- All test cases for the assignment should be passing.
|
|
||
| switch = 0 | ||
| while switch == 0 && i < array_one.length | ||
| array_one[i] != array_two[i] ? switch = 1 : i += 1 |
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 is absolutely a correct implementation of this loop, but it's a little surprising. Normally in imperative languages like Ruby, I expect the ternary operator (the ? : syntax) to be used when assigning variables, but you aren't assigning the result of this operator to anything. As someone who likes to operate on the principle of least surprise, I'd find it more obvious if I read:
if (array_one[i] != array_two[i])
return false
end
i += 1That also eliminates your need for the switch variable.
| # # solution 2 --> It loops through the whole array and then compares the number | ||
| # of assertions to the length. | ||
|
|
||
| # assertion = 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.
Just as a note, it's better not to leave commented-out code in committed files. However, in this case, it looks like you wanted us to see the second solution you came up with. Since it's here, I'll say: solution 1 is the better solution because it has the ability to stop looping as soon as it finds an element. The opportunity for algorithms to take shortcuts like that are things you should be looking for, so I'm glad you chose to leave solution 1 and not solution 2 in the code. :)
| first_array_size = rand(1..2) | ||
| second_array_size = rand(1..2) | ||
|
|
||
| first_array = [] |
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.
It's great to see you testing and trying out your code! Ideally, you shouldn't leave that code in this file, though. Don't feel afraid of adding test cases to the spec files when it makes sense as you work, and also don't feel afraid of adding a .rb file next to this one where this sort of code can live (and where you could run it separately).
:)