Skip to content

Add tests for write! edge cases in Circular Buffer#124

Merged
kytrinyx merged 1 commit intoexercism:masterfrom
julianandrews:add-edge-case-tests-to-circular-buffer
May 5, 2015
Merged

Add tests for write! edge cases in Circular Buffer#124
kytrinyx merged 1 commit intoexercism:masterfrom
julianandrews:add-edge-case-tests-to-circular-buffer

Conversation

@julianandrews
Copy link
Copy Markdown
Contributor

Tests for "CircularBuffer.write!" with "nil" argument and on non-full
buffers. Fixes #123.

Tests for "CircularBuffer.write!" with "nil" argument and on non-full
buffers. Fixes #123.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is actually an interesting question overall... it seems odd to me now that a nil would be treated differently than any another value, but I see that we're already doing this for the write with a nil.

Let's do this for now, but also be open to having the discussion about what nil actually means in the context of a circular buffer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had the a similar thought. It's notable that all of the test cases only use strings as values for the buffer. If the intent is a circular buffer for strings only, then probably instead of silently failing on a nil write, the buffer should throw an exception on any non-string value.

If instead the intent is a buffer that can accept arbitrary objects, then nil should be a valid thing to write. Of course, that would make the exercise a notch more difficult since the simple approach of using a nil filled array wouldn't work anymore, but it would also be more general an correct.

Since the buffer is suppose to raise an error if you try to read from it empty, there's no scenario where nil is used to represent an empty value or anything like that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, I like the idea of making this a circular string buffer, and to have any non-string values raise errors. We could have several tests to that effect (nil, fixnum, Object.new, whatever), or one test with several different types of things.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could write a fix to replace the nil write tests with tests that check a representative set of non-string values all throw an ArgumentError (or some custom exception). In that case it might also make sense to have some of the tests test longer strings (not just single character strings) just to make it clear that any strings (but only strings) are valid inputs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, longer strings would help clarify.

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented May 5, 2015

Thanks!

kytrinyx added a commit that referenced this pull request May 5, 2015
…cular-buffer

Add tests for write! edge cases in Circular Buffer
@kytrinyx kytrinyx merged commit 71c4e99 into exercism:master May 5, 2015
gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016
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.

Missing edge cases in Circular Buffer tests.

2 participants