-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: master
Are you sure you want to change the base?
Sockets - Erica #33
Conversation
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. :) |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
No description provided.