Skip to content

Conversation

@laneia
Copy link

@laneia laneia commented Feb 28, 2019

No description provided.

@laneia laneia changed the title passing all tests Ports - Laneia Feb 28, 2019
@kdow
Copy link

kdow commented Mar 2, 2019

Looks good! The only suggestion I have is you might want to consider changing the i variable to have a descriptive name.

Copy link

@ace-n ace-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few style nits, but passes tests + LGTM overall. Nice work! 👍

Also, +1 to what @kdow said. 😃

# and the same integer values in the same exact order
def array_equals(array1, array2)
raise NotImplementedError
i = 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can probably be declared closer to where it's used 😄

raise NotImplementedError
i = 0

if (array1 == nil && array2 == nil) || (array1 == [] && array2 == [])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good boundary checking!

Super-tiny nit: it's somewhat explicit though - could we do something more concise, like:

 if (array1 == array2)
   return true
 end

(Aside: some people think explicit code is easier to read, while
others prefer more concise code - so this comment is really a
matter of personal preference. 😄 )

return false
end

if array1.length == array2.length
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nested ifs are harder to understand at a glance. How about something like:

if array1.length != array2.length
  return false
end 

# array lengths presumed equal, because of above "guarding" if statement

...

if array1[i] != array2[i]
return false
end
i += 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit: typically people use a "for" loop here rather than manually doing i += 1 (it's more concise, which makes it easier to identify at a glance)

return false
end

array3 = [10, 20, 30, 40, 50, 60]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice testing! 😄

Aside: typically we'll put our tests in a separate file. This is
important because some languages (e.g. Python and Javascript) automatically
execute top-level code in loaded files/libraries.

e.g. if fileB imports fileA, top-level code (that's not in a function)
in fileA will automatically be executed in such languages. (In some cases, this is what you want - in others, it isn't.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants