Skip to content

Fixed LinkedList: #662

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 7 commits into from
Nov 30, 2017
Merged

Fixed LinkedList: #662

merged 7 commits into from
Nov 30, 2017

Conversation

yossan
Copy link

@yossan yossan commented Nov 25, 2017

Checklist

  • [*] I've looked at the contribution guidelines.
  • [*] This pull request is complete and ready for review.

Description

  • Modified node(atIndex:) method not to return nil, and code refactoring.
  • Deleted nodesBeforeAndAfter(index:) method, and modified insert methods.
  • Modified readme file.

* Modified node(atIndex:) method not to return nil, and code refactoring.
* Deleted nodesBeforeAndAfter(index:) method, and modified insert methods.
@hollance
Copy link
Member

How about 'guard var node = head { ... }'. This saves having to write 'var node = head' below the guard statement.

@yossan
Copy link
Author

yossan commented Nov 26, 2017

Thanks for your suggestion.
I adopted it.
dc810d2

@yossan
Copy link
Author

yossan commented Nov 29, 2017

I apologize for having sent a lot of commits after pull request.
I would be grateful if you would review this pull request.

Copy link
Member

@kelvinlauKL kelvinlauKL left a comment

Choose a reason for hiding this comment

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

Nice changes overall. I left a comment at one of the many signatures with the following style:

node(atIndex:)

Changing it to node(at index: Int) would read nicer at the call site: node(at: 10), and fits in better with the ones in the collection apis.

if i == 0 { return node }
i -= 1
node = node!.next
public func node(atIndex index: Int) -> Node {
Copy link
Member

Choose a reason for hiding this comment

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

node(at index: Int) would read a bit more like the other collection apis.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I renamed atIndex to at by the following methods:

  • node(atIndex:)
  • insert(_:, atIndex:)
  • remove(atIndex:)

bb21f61

Copy link
Member

@kelvinlauKL kelvinlauKL left a comment

Choose a reason for hiding this comment

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

Good changes!

@kelvinlauKL
Copy link
Member

Thank you @ysn551!

@kelvinlauKL kelvinlauKL merged commit a5d3185 into kodecocodes:master Nov 30, 2017
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