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 - Maria #48

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

Conversation

MariaWissler
Copy link

No description provided.

@Kimberly-Fasbender
Copy link

Kimberly-Fasbender commented Mar 5, 2019

I really liked that you used a counter to keep track of how many indexes to iterate through for an unknown array length! My only suggestion would be that you might want to consider moving the variable "index" (which stores the index counter) down to line 34, so it is closer to the conditional statement that is utilizing it. Your code is very readable, and I didn't have any problems figuring out what was happening in it! Great job!

Copy link

@olenellina olenellina left a comment

Choose a reason for hiding this comment

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

I really appreciate your use of comments; it's very helpful when submitting a review to your peers and helps with code documentation. Your code is concise and readable, and also includes good checks for edge cases. I agree with the other commenter that you could move the index variable closer to where it's being used but minor and overall great job!

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