A Generic Two Dimensional Data Plane with SubPlane Method in C#

Question

This is a follow-up question for Generic Two Dimensional Data Plane with Manipulation Methods For C# and A SubPlane Method for Generic Two Dimensional Data Plane in C#. Based on iSR5's suggestion, in SubPlane method, Array.Copy is used instead of regular for loop in order to improve performance.

The experimental implementation

The experimental implementation is as below.

[Serializable]
public class PlaneV3<T>
{
    public int Width { get; }

    public int Height { get; }

    public T[,] Grid { get; set; }

    public int Length => Width * Height;

    public PlaneV3() : this(1, 1) { }

    public PlaneV3(int width, int height)
    {
        Width = width;
        Height = height;
        Grid = new T[width, height];
    }

    public PlaneV3(PlaneV3<T> plane) : this(plane?.Grid) { }

    public PlaneV3(T[,] sourceGrid)
    {
        if (sourceGrid is null)
        {
            throw new ArgumentNullException();
        }

        Width = sourceGrid.GetLength(0);
        Height = sourceGrid.GetLength(1);
        Grid = new T[Width, Height];
        Array.Copy(sourceGrid, Grid, sourceGrid.Length);
    }

    public PlaneV3<T> ConcatenateHorizontal(PlaneV3<T> input)
    {
        if (input is null || input.Grid is null)
        {
            throw new ArgumentNullException();
        }

        if (this.Height.Equals(input.Height).Equals(false))
        {
            throw new ArgumentException();
        }

        var output = new PlaneV3<T>(this.Width + input.Width, this.Height);
        for (int x = 0; x < this.Width; x++)
        {
            Array.Copy(this.Grid, x * this.Height, output.Grid, x * output.Height, this.Height);
        }
        for (int y = 0; y < this.Height; y++)
        {
            for (int x = 0; x < input.Width; x++)
            {
                output.Grid[this.Width + x, y] = input.Grid[x, y];
            }
        }
        return output;
    }

    public PlaneV3<T> ConcatenateHorizontalInverse(PlaneV3<T> input)
    {
        return input.ConcatenateHorizontal(this);
    }

    public override bool Equals(object obj)
    {
        var input = obj as PlaneV3<T>;
        //  https://stackoverflow.com/a/12446807/6667035
        return this.Grid.Rank == input.Grid.Rank &&
                System.Linq.Enumerable.Range(0, this.Grid.Rank).All(dimension => this.Grid.GetLength(dimension) == input.Grid.GetLength(dimension)) &&
                this.Grid.Cast<T>().SequenceEqual(input.Grid.Cast<T>());
    }

    public PlaneV3<T> SubPlane(int locationX, int locationY, int newWidth, int newHeight)
    {
        if (this.Grid == null)
        {
            return null;
        }
        PlaneV3<T> outputPlaneV3 = new PlaneV3<T>(newWidth, newHeight);
        for (int y = 0; y < newHeight; y++)
        {
            Array.Copy(this.Grid, (y + locationY) * this.Width + locationX, outputPlaneV3.Grid, y * newWidth, newWidth);
        }
        return outputPlaneV3;
    }

    public override string ToString() => $"{nameof(PlaneV3<T>)}<{typeof(T).Name}>[{Width}, {Height}]";

    public string ToDelimitedStringByHeightThenWidth(string separator = "\t")
    {
        return ToDelimitedString(Height, Width, separator, true);
    }

    public string ToDelimitedStringByWidthThenHeight(string separator = "\t")
    {
        return ToDelimitedString(Width, Height, separator, false);
    }

    private string ToDelimitedString(int outterCounter, int innerCounter, string separator, bool isHeightThenWidth)
    {
        var lines = new StringBuilder();
        var columns = new StringBuilder();

        for (int x = 0; x < outterCounter; x++)
        {
            var columnDelimiter = string.IsNullOrWhiteSpace(separator) ? "" : separator;
            for (int y = 0; y < innerCounter; y++)
            {
                if (isHeightThenWidth)
                {
                    columns.Append(columnDelimiter + Grid[y, x].ToString());
                }
                else
                {
                    columns.Append(columnDelimiter + Grid[x, y].ToString());
                }

                if (columnDelimiter == string.Empty) // more readability 
                {
                    columnDelimiter = string.IsNullOrWhiteSpace(separator) ? "\t" : separator;
                }
            }

            lines.AppendLine(columns.ToString());
            columns.Clear(); // Clear and reuse it instead of initiating a new object.
        }

        return lines.ToString();
    }

} // class

Test cases

The usage cases of PlaneV3 class above are as follows.

PlaneV3<string> planeV3_1 = new PlaneV3<string>(6, 6);
for (int j = 0; j < 6; j++)
{
    for (int i = 0; i < 6; i++)
    {
        planeV3_1.Grid[i, j] = "(" + i + "," + j + ")";
    }
}
Console.WriteLine(planeV3_1.SubPlane(1, 1, 3, 3).ToDelimitedStringByHeightThenWidth());
Console.WriteLine("");
Console.WriteLine(planeV3_1.SubPlane(1, 1, 3, 3).ToDelimitedStringByWidthThenHeight());

PlaneV3<string> planeV3_2 = new PlaneV3<string>(6, 6);
for (int j = 0; j < 6; j++)
{
    for (int i = 0; i < 6; i++)
    {
        planeV3_2.Grid[i, j] = "(" + i + "," + j + ")";
    }
}
Console.WriteLine(planeV3_1.Equals(planeV3_2).ToString());

All suggestions are welcome.

The summary information:

• Which question it is a follow-up to?

  Generic Two Dimensional Data Plane with Manipulation Methods For C# and

  A SubPlane Method for Generic Two Dimensional Data Plane in C#

• What changes has been made in the code since last question?

  • In SubPlane method, Array.Copy is used instead of regular for loop.

  • Trying to implement ConcatenateHorizontal based on Array.Copy method.

• Why a new review is being asked for?

  If there is any possible improvement or any other potential defect, please let me know.

Answer 1

Major mistake Grid = new T[width, height];, must be Grid = new T[height, width];. Maybe better to use Rows and Columns e.g. new T[rows, columns]? In further code you made a lot of bugs related to this issue.

Properties

• First of all make the Grid immutable. It looks like developer can replace the array with any other one.
• Every time I ask Length, it will be calculated from with and height. It can be done only once in constructor.

ConcatenateHorizontal

• Checking input.Grid is null is a kind of redundancy. I can't imagine the scenario.
• throw new ArgumentNullException(nameof(input)) - pass the name of the argument to exception, it would be easier to debug that, especially in methods with multiple arguments. And/or pass an exception message text.
• if (someBool.Equals(false)) can be replaced with if (!someBool).
• A typo in Array.Copy(this.Grid, x * this.Height, output.Grid, x * output.Height, this.Height); the last argument must be must be this.Width? Guessing that caused by Width and Height issue.
• i, j or x,y can mislead you when you work with arrays, use row,col because it sound the same as you think about it.
• You changed one inner loop or Array.Copy, you can change both.

Equals

• obj can be null or wrong type, you missed a check. Also better to use pattern matching with assignment here.
• this.Grid.Rank == input.Grid.Rank is redundant check.
• System.Linq.Enumerable.Range - better to add using System.Linq; directive and write simply Enumerable.Range but it's faster and simpler to check Width and Height because you have known amount of dimensions and only two.

SubPlane

• if (this.Grid == null) - it can't be null.
• Missed checking of arguments to fit the source area.

ToString

• Again Height and Width

WithThenHeight and HeightThenWidth

• Probably these methods created after Width and Height issue. Make a string as the array is with exact rows and columns count. I removed these methods as redundant ones.

ToDelimitedString

• One StringBuilder is enough
• A kind of columns.Append(columnDelimiter + Grid[y, x].ToString()); can be replaced with columns.Append(columnDelimiter).Append(Grid[y, x].ToString()); because StringBuilder is intended to replace string concatenations with rather faster solution.
• You have arguments for size of output section but no arguments for its location. I remove that arguments as inconsistent with data.
• Why not Space as default separator? For readability. Also you may calculate a padding to avoid using \t as unstable solution e.g. you need different amount of \t from one to align 123456789012 and 0. Let's make it smarter.

[Serializable]
public class PlaneV3<T>
{
    public int Width { get; }

    public int Height { get; }

    public T[,] Grid { get; }

    public int Length { get; }

    public PlaneV3() : this(1, 1) { }

    public PlaneV3(int width, int height)
    {
        Width = width;
        Height = height;
        Grid = new T[height, width];
        Length = Width * Height;
    }

    public PlaneV3(PlaneV3<T> plane) : this(plane?.Grid ?? throw new ArgumentNullException(nameof(plane))) { }

    public PlaneV3(T[,] sourceGrid)
    {
        if (sourceGrid is null)
        {
            throw new ArgumentNullException(nameof(sourceGrid));
        }

        Height = sourceGrid.GetLength(0);
        Width = sourceGrid.GetLength(1);
        Length = Width * Height;
        Grid = new T[Height, Width];
        Array.Copy(sourceGrid, Grid, sourceGrid.Length);
    }

    public PlaneV3<T> ConcatenateHorizontal(PlaneV3<T> input)
    {
        if (input is null)
        {
            throw new ArgumentNullException(nameof(input));
        }

        if (this.Height != input.Height)
        {
            throw new ArgumentException("Height isn't match", nameof(input.Height));
        }

        var output = new PlaneV3<T>(this.Width + input.Width, this.Height);
        for (int row = 0; row < this.Height; row++)
        {
            Array.Copy(this.Grid, row * this.Width, output.Grid, row * output.Width, this.Width);
            Array.Copy(input.Grid, row * input.Width, output.Grid, row * output.Width + this.Width, input.Width);
        }

        return output;
    }

    public PlaneV3<T> ConcatenateHorizontalInverse(PlaneV3<T> input)
    {
        return input.ConcatenateHorizontal(this);
    }

    public override bool Equals(object obj)
    {
        return obj is PlaneV3<T> input && this.Width == input.Width && this.Height == input.Height && this.Grid.Cast<T>().SequenceEqual(input.Grid.Cast<T>());
    }

    public PlaneV3<T> SubPlane(int locationX, int locationY, int newWidth, int newHeight)
    {
        if (locationX < 0 || locationX >= this.Width)
             throw new ArgumentException("Location must be inside the area", nameof(locationX));
        if (locationY < 0 || locationY >= this.Height)
             throw new ArgumentException("Location must be inside the area", nameof(locationY));
        // perform other similar checks here if needed

        PlaneV3<T> outputPlaneV3 = new PlaneV3<T>(newWidth, newHeight);
        for (int row = 0; row < newHeight; row++)
        {
            Array.Copy(this.Grid, (row + locationY) * this.Width + locationX, outputPlaneV3.Grid, row * newWidth, newWidth);
        }
        return outputPlaneV3;
    }

    public override string ToString() => $"{nameof(PlaneV3<T>)}<{typeof(T).Name}>[{Height}, {Width}]";

    public string ToDelimitedString(string separator = " ")
    {
        var sb = new StringBuilder();
        string[,] output = new string[this.Height, this.Width];
        int maxLength = 0;

        for (int row = 0; row < this.Height; row++)
        {
            for (int column = 0; column < this.Width; column++)
            {
                string s = this.Grid[row, column].ToString();
                output[row, column] = s;
                maxLength = Math.Max(maxLength, s.Length);
            }
        }
        
        for (int row = 0; row < this.Height; row++)
        {
            for (int column = 0; column < this.Width; column++)
            {
                sb.Append(output[row, column].PadLeft(maxLength)).Append(separator);
            }
            sb.Remove(sb.Length - separator.Length, separator.Length).AppendLine();
        }
        
        return sb.ToString();
    }
}