Skip to content

Conversation

@norrise120
Copy link

No description provided.

@lebaongoc
Copy link

I think the nested "if" structure (an if statement within an if statement -lines 4,5 and an elseif -line 10, etc.) can become difficult to track with these many boolean expressions. I would avoid nested "if" structure if possible and end the if statement when needed. :)

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.

def array_equals(array1, array2)
raise NotImplementedError
if array1 == nil || array2 == nil
if array1 == nil && array2 == nil

Choose a reason for hiding this comment

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

This whole block takes the form of:

if (condition) return true else return false end

Simplify to: return (condition)

The latter is more readable and easier to type.

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

Choose a reason for hiding this comment

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

The array.length.times method replaces the value of i with whatever comes next, so this statement is redundant and has no real effect. Remove.

if array1.length == 0
return true
else
i = 0

Choose a reason for hiding this comment

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

Unnecessary initialization.

else
return false
end
end

Choose a reason for hiding this comment

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

General comment: blank lines are your friend. Add some whitespace between logical blocks of code.

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