Skip to content

Add new exercise simple linked list #99

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

Merged
merged 1 commit into from
Mar 6, 2016

Conversation

matthewmorgan
Copy link
Contributor

As described in #98

@sit
Copy link
Contributor

sit commented Mar 1, 2016

Why both this one and #85?


/**
*
* @author matt
Copy link
Contributor

Choose a reason for hiding this comment

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

You can teach your IDE not to generate these comments. They probably aren't relevant for this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point.

@matthewmorgan
Copy link
Contributor Author

@sit "linked-list" is supposed to be doubly linked. This exercise is singly linked. Both are specified in x-common.

Edit: @sit interestingly, the x-common README for 'linked-list' says it should be doubly linked, but the .yml says it should be singly linked! Maybe that needs to be addressed next.

@matthewmorgan matthewmorgan force-pushed the new-exercise-simple-linked-list branch 3 times, most recently from 7a2575f to ada43f8 Compare March 2, 2016 12:01
@matthewmorgan
Copy link
Contributor Author

Refactored to allow generics. Feedback appreciated!

return (T[]) result;
}

private <T> T[] getArray(Class<T> clazz, int size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

newArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting changing the method name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, to indicate that it's constructing something (as opposed to getting something).

@matthewmorgan matthewmorgan force-pushed the new-exercise-simple-linked-list branch from ada43f8 to c4b10d7 Compare March 3, 2016 15:42
@matthewmorgan
Copy link
Contributor Author

@sit @jtigger is this OK to merge? Any remaining concerns? Thanks

@sit
Copy link
Contributor

sit commented Mar 6, 2016

LGTM.

kytrinyx added a commit that referenced this pull request Mar 6, 2016
@kytrinyx kytrinyx merged commit b03f4f1 into master Mar 6, 2016
@kytrinyx
Copy link
Member

kytrinyx commented Mar 6, 2016

Thanks!

@matthewmorgan matthewmorgan deleted the new-exercise-simple-linked-list branch March 6, 2016 20:44
@amrtanair amrtanair mentioned this pull request Mar 7, 2016
@jtigger
Copy link
Contributor

jtigger commented Mar 8, 2016

btw, @sit, we were singing your praises, this morning. Great spot
feedback/questions! Thank you, sir! Hoping to help carry forward that
which you started.

On Sun, Mar 6, 2016 at 11:53 AM, Katrina Owen [email protected]
wrote:

Thanks!


Reply to this email directly or view it on GitHub
#99 (comment).

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.

4 participants