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

Linked List: how to handle the empty list in pop and shift? #286

Closed
balazsbotond opened this issue Jan 21, 2017 · 5 comments
Closed

Linked List: how to handle the empty list in pop and shift? #286

balazsbotond opened this issue Jan 21, 2017 · 5 comments

Comments

@balazsbotond
Copy link
Contributor

There is currently no unit test checking what pop and shift do when the list is empty. I wonder if returning an option would be a better way to handle this case instead of returning the value if present or throwing an exception. Though Map has separate find and tryFind methods so I'm not sure if this is idiomatic.

I think adding tests for the empty case would be a good idea in any case and if you agree, I can write them. And if you think option is the way to go, I will change that too.

@ErikSchierboom
Copy link
Member

I agree that testing the empty case would be a good idea. Returning an option value is also more idiomatic, so it would be great if you could also change that to

@balazsbotond
Copy link
Contributor Author

Cool, thanks! I'll send those changes later today.

@robkeim
Copy link
Contributor

robkeim commented Jan 23, 2017

Sorry I read the PR before seeing there was a related issue that I should have commented on...

Copying my comment here:

I agree this is more idiomatic, but from the description.md it says:

To keep your implementation simple, the tests will not cover error conditions. Specifically: pop or shift will never be called on an empty list.
Shouldn't we stay aligned with what's in x-common?

@robkeim robkeim reopened this Jan 23, 2017
@balazsbotond
Copy link
Contributor Author

Sorry, I haven't read the README carefully enough. I agree with @robkeim, I think my pull request should be reverted.

@ErikSchierboom
Copy link
Member

Whoops. Totally missed that. A retraction is then indeed better

ErikSchierboom pushed a commit that referenced this issue Feb 5, 2017
* Fix #286 - Linked List: change pop and shift to return option

* Linked List: Add missing Ignore attribute

* Markdown: Use correct tags for italic and bold text

* Revert "Fix #286 - Linked List: change pop and shift to return option"

This reverts commit 9da0115.
robkeim pushed a commit that referenced this issue Feb 9, 2017
#300)

* Fix #286 - Linked List: change pop and shift to return option

* Linked List: Add missing Ignore attribute

* Markdown: Use correct tags for italic and bold text

* Revert "Fix #286 - Linked List: change pop and shift to return option"

This reverts commit 9da0115.

* atbash-cipher: Replace [<TestCase>] tests with individual tests (#298)

* Add missing [<Test>] attribute

* Add missing [<Ignore>] attributes
robkeim pushed a commit that referenced this issue Feb 9, 2017
* Fix #286 - Linked List: change pop and shift to return option

* Linked List: Add missing Ignore attribute

* Markdown: Use correct tags for italic and bold text

* Revert "Fix #286 - Linked List: change pop and shift to return option"

This reverts commit 9da0115.

* Acronym: Replace [<TestCase>] tests with individual tests (#298)

* acronym: Add missing [<TestCase>] attribute

* Add missing [<Ignore>] attributes
robkeim pushed a commit that referenced this issue Feb 9, 2017
* Fix #286 - Linked List: change pop and shift to return option

* Linked List: Add missing Ignore attribute

* Markdown: Use correct tags for italic and bold text

* Revert "Fix #286 - Linked List: change pop and shift to return option"

This reverts commit 9da0115.

* clock: Replace [<TestCase>] tests with individual tests (#298)
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

No branches or pull requests

3 participants