Skip to content
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

Sockets - Erica #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sockets - Erica #33

wants to merge 1 commit into from

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.

@@ -1,5 +1,26 @@
# 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 == 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.

end
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