RoundStack Implementation

I just recently made an attempt at implementing what I have been led to understand is called a RoundStack, simply meaning contrary to .NET Stack's default behavior of doubling internal array size when it hits capacity, simply starts dropping off items at the bottom of the stack. I am developing this class for the purpose of facilitating an undo/redo system. I wanted to make this compliant with the functionality the default .NET stack has, and thus borrowed several pieces of code from the .NET reference source, only modifying where absolutely necessary to facilitate the round-robin pushing.

I have written several test in NUnit and the collection is working as intended. Now looking for any comments as to whether I implemented this properly and in the most efficient way.

```
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Schloss.Collections.Generic
{
[Serializable]
public class RoundStack : IEnumerable, ICollection, IEnumerable
{
private T[] _items; // items.Length is Capacity + 1
private int _size;
private int _version;

// if top is equal to bottom, stack is full
private int _top = 1;
private int _bottom = 0;

private Object _syncRoot;

private const int _defaultCapacity = 4;
static T[] _emptyArray = new T[0];

public RoundStack()
{
_items =_emptyArray;
_size = 0;
_version = 0;
}

public RoundStack(int capacity)
{
if (capacity collection)
{
if (collection == null)
throw new ArgumentNullException("Cannot create from a null collection.");

// borrowed from .NET Reference code for System.Collections.Generic.Stack
ICollection c = collection as ICollection;
if (c != null)
{
int count = c.Count;
_items = new T[count];
c.CopyTo(_items, 0);
_size = count;
}
else

I like the exceptions you're throwing in your guard clauses, you're throwing exception types the client code would rightfully expect in these circumstances.

Vertical whitespace isn't consistent though. I like this:

if (collection == null)
        throw new ArgumentNullException("Cannot create from a null collection.");

    // borrowed from .NET Reference code for System.Collections.Generic.Stack
    ICollection c = collection as ICollection;

But for consistency this one should also have an empty line after the guard clause:

if (capacity < 1)
        throw new ArgumentOutOfRangeException("Capacity must be at least one.");
    _items = new T[capacity + 1];

You're not very consistent either with one-liner scope braces - you omit them in guard clauses (which is fine IMO), but then there's this:

if (_size < 0)
            _size += _items.Length;

And then that:

if (_syncRoot == null)
        {
            System.Threading.Interlocked.CompareExchange(ref _syncRoot, new Object(), null);
        }

As a maintainer I would much rather like to see all scopes properly enclosed in curly braces.

// borrowed from .NET reference
    int count = _size;

That's not really a useful comment. This would be more useful:

// thread-local copy:
    int count = _size;

In the event that item is a value type, this would be boxing it:

if (((Object)item) == null)

One of the advantages of generics, is that boxing becomes avoidable, since you have a way of knowing whether T is a value or a reference type. Why not leverage this knowledge?

In fact, the whole Contains implementation smells:

if (((Object)item) == null)
        {
            if (((Object)_items[count]) == null)
                return true;
        }
        else if (_items[count] != null && c.Equals(_items[count], item))
        {
            return true;
        }

Can be simplified to:

if ((object)item == null && (object)_items[count] == null)
        {
            return true;
        }
        else if (_items[count] != null && c.Equals(_items[count], item))
        {
            return true;
        }

And that still smells... but contracting the conditions any further would definitely hinder readability.

Notice I replaced Object with object - object is a language alias for System.Object (which you are using). For consistency's sake, if you're going to be using C# aliases for CLR types (object, int, string, ...), then use aliases throughout the code. Or don't, and use the CLR types everywhere (Object, Int32, String, ...).

This is MASSIVE overkill:

using (IEnumerator en = collection.GetEnumerator())
        {
            while (en.MoveNext())
            {
                Push(en.Current);
            }
        }

You know that collection is an IEnumerable - why not just iterate it with a more casual-looking foreach loop?

foreach(var item in collection)
        {
            Push(item);
        }

StackExchange Code Review Q#78023, answer score: 4