Thread-safe implementation of ReturnMultipleValues.

[jbialobr]

Fixes #282.

[zvirja]

@jbialobr Please rebase your code on top of the latest master - issues with CI were fixed there.

I've checked the suggested implementation and while it looks correct, this index arithmetic looks a bit messy to understand :)

Wouldn't it be much easier to use the ConcurrentQueue<> instead? So code will look like:

public ReturnMultipleValues(T[] values)
{
	_lastValue = values.LastOrDefault();
	_valuesToReturn = new ConcurrentQueue<T>();
}

internal T GetNext()
{
	if (_valuesToReturn.TryDequeue(out T nextResult))
	{
		return nextResult;
	}
	else
	{
		return _lastValue;
	}
}

But of course, your code might work quicker and have less overhead for runtime - so it's a question :)

[jbialobr]

But of course, your code might work quicker and have less overhead for runtime - so it's a question :)

Yes, you are right, we should not see any difference in performance while performing a typical unit test.

Rebased and replaced with ConcurrentQueue.

[zvirja]

Please pass an IEnumerable here and later make array in constructor if you need that. It's implementation detail that IEnumerables should be converted to arrays.

Moreover, we don't even need that - just initialize ConcurrentQueue with IEnumerable and later call LastOrDefault() on the created queue.

We can do the same for function overload :)

[jbialobr]

I think that since ReturnMultipleValues is backed by ConcurrentQueue it should not accept IEnumerable as an argument. You can create an IEnumerable instance that produces infinite number of values.

[zvirja]

Very interesting concern :)

I believe that should not be the case as I'd consider infinite enumerables as something really awful in design. If that would be a real concern, we would not be able to consume IEnumerables anywhere in the code as we often convert IEnumerables to arrays. We always consider enumerables as finite sequences.
Another nice answer on SO why infinite enumerables are evil.

But let's see what @alexandrnikitin or @dtchepak think about this.

[alexandrnikitin]

I prefer to limit the definition to more strict types, array in this case. IEnumerable<T> means not-materialized-in-memory/ undefined stream of data. The method taking IEnumerable<T> as an argument should take that into account and be lazy all the way down. We have finite and defined number of arguments in our case.

[zvirja]

Thanks for your reply.
My concern was actually that we create redundant array which is abandoned immediately. I thought that it doesn't make sense to spam objects in places where we could easily omit that via design.

Nevertheless, this extension method is invoked rarely, so we could disregard that :)

[alexandrnikitin]

Well, regarding performance, there are other bottlenecks we can address ;) For instance #248

[zvirja]

Could be simplified to Enumerable.Range(..).ToArray()

[zvirja]

Please add one more test to test that the last value is returned if you ask more values than configured.

[jbialobr]

Added an additional assertion for that.

[zvirja]

Thank you very much! May I ask you to create a separate test for that instead of extending the current one? It isn't a very good practice to have have multiple different assertions within the single test - it's harder to read them.

Also, if you have a free time, it would be fine to test both Returns(value1, value2) and Returns(c => .., c => ...) overloads as you changed both them.

Thank you.

[jbialobr]

It isn't a very good practice to have have multiple different assertions within the single test - it's harder to read them.

I would say that it is not a good practice to have multiple different assertions within a single test when the additional assertions make the test harder to read it.

[zvirja]

ok, let's keep that up to @alexandrnikitin and @dtchepak.

[alexandrnikitin]

@zvirja I think it's enough to have only multi-threaded test in our case. We have pletty of tests for Returns(...)

[alexandrnikitin]

Janusz, thank you for the PR! Overall looks good to me. Just two comments from my side. I want to have more readable test. I didn't like the way you coupled ReturnMultipleValues and ReturnMultipleFuncsValues. They share only dequeuing logic, I don't see a reason to tie types to each other. I address the comments in jbialobr#1 Please TAL

[dtchepak]

Merged in, thanks a lot @jbialobr! Thanks @alexandrnikitin and @zvirja too for the PR review work.

[dtchepak]

Released in 2.0.3.