Ports - Mina #24
Ports - Mina #24
Conversation
|
I love how you group similar ideas together (e.g. checking for empty array on the same line as checking for nil). Great job! |
There was a problem hiding this comment.
Nice job!
Checklist
- Clean, working code
- Efficient code - give feedback as you would give to a peer on your team
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.
| @@ -1,5 +1,16 @@ | |||
| # Determines if the two input arrays have the same count of elements | |||
| # and the same integer values in the same exact order | |||
| def array_equals(array1, array2) | |||
| raise NotImplementedError | |||
| if (array1 == [] && array2 == [] || array1 == nil && array2 == nil) | |||
There was a problem hiding this comment.
[nit] Technically, comparing with [] relies on the built-in array equality operator. It's also redundant with checking length, since if both arrays are equals to empty arrays, they will both have length of 0. You should be able to safely remove array1 == [] && array2 == [] || .
| return true | ||
| elsif array1 == nil || array2 == nil || array1.length != array2.length | ||
| return false | ||
| else |
There was a problem hiding this comment.
Not-actual-feedback industry experience note:
It's not 100% necessary to use elsif and else for this particular set of logic because of the fact that you're returning early. The early return naturally excludes any logic that happens later in the function.
You could argue, and I tend to lean that way, that it's better style (and more future-proof) to keep the if/elsif/else structure, just as you have it. At the same time, you may encounter (as I have at work) some IDEs or tools that enforce removing these clauses from the statements they surround when early returns exist and they're not absolutely necessary.
I wouldn't change what you have, but I thought you'd appreciate knowing that some shops will consider this style "wrong" and some shops will consider this style "right". It's something you'll find out and adapt to when you get there.
| elsif array1 == nil || array2 == nil || array1.length != array2.length | ||
| return false | ||
| else | ||
| array1.length.times do |item| |
There was a problem hiding this comment.
[nit] Using the name item for an array index is a little confusing. If I'm reading this quickly, I might see |item| and assume that it's preceded by array1.each, rather than a times loop. It's wholly appropriate to call this variable i or idx, since those are pretty common names for a variable holding an index you're using to iterate through collections.
| raise NotImplementedError | ||
| if (array1 == [] && array2 == [] || array1 == nil && array2 == nil) | ||
| return true | ||
| elsif array1 == nil || array2 == nil || array1.length != array2.length |
There was a problem hiding this comment.
Not-actual-feedback industry experience note:
It's not 100% necessary to use elsif and else for this particular set of logic because of the fact that you're returning early. The early return naturally excludes any logic that happens later in the function.
You could argue, and I tend to lean that way, that it's better style (and more future-proof) to keep the if/elsif/else structure, just as you have it. At the same time, you may encounter (as I have at work) some IDEs or tools that enforce removing these clauses from the statements they surround when early returns exist and they're not absolutely necessary.
I wouldn't change what you have, but I thought you'd appreciate knowing that some shops will consider this style "wrong" and some shops will consider this style "right". It's something you'll find out and adapt to when you get there.
No description provided.