Skip to content

Conversation

@cyndilopez
Copy link

No description provided.

@kaseea
Copy link

kaseea commented Mar 4, 2019

very clean looking! I think you missed the case for nil and empty array though.

Copy link

@eric-andeen eric-andeen left a comment

Choose a reason for hiding this comment

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

I'm Eric. I'll be your volunteer code reviewer. I've been a software developer for about a zillion years. I'll be applying most of the same professional standards to your code as I do in the day job. Some of my comments may seem pedantic, persnickety, or irrelevant to the kinds of exercises you're doing in class, but once you start your internship, you'll get the same kind of feedback I'm giving you here.

Good code for your first assignment.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
Copy link

@eric-andeen eric-andeen Apr 9, 2019

Choose a reason for hiding this comment

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

Parameter names are inconsistent: arr_1 and array_2.
Parameter names are also unnecessarily verbose. array1 and array2 (as in the problem statement) would be fine.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
arr_integers_1.length.times do |index|

Choose a reason for hiding this comment

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

Need to handle the cases of array1 == nil, array2 == nil, array1.length != array2.length. This statement DOES cover zero-length arrays.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
arr_integers_1.length.times do |index|
return false if arr_integers_1[index] - array_integers_2[index] != 0

Choose a reason for hiding this comment

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

a != b is much clearer than a - b != 0, even if mathematically equivalent.

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