1
\$\begingroup\$

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:

\$\endgroup\$

1 Answer 1

0
\$\begingroup\$

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();
    }
} 
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Thank you for the answer. Wondering why T[height, width], or T[rows, colums]. Is it because of the sequence of data storage? \$\endgroup\$
    – JimmyHu
    Commented Jan 22, 2021 at 0:52
  • 1
    \$\begingroup\$ @JimmyHu yes, exactly. Let me add few more improvements to the code. \$\endgroup\$
    – aepot
    Commented Jan 22, 2021 at 0:53
  • 1
    \$\begingroup\$ I appreciate your answer! Let me check it :) \$\endgroup\$
    – JimmyHu
    Commented Jan 22, 2021 at 1:00
  • 1
    \$\begingroup\$ @JimmyHu I didn't test it because i wrote the code from my mobile phone, far away from nearest PC. :) Sorry if some typos there. As further ToDelimitedString improvement you can calculate maxLength separately for each column and store to some int[Width] array to make the output more compact in some cases. The idea is very easy to catch. \$\endgroup\$
    – aepot
    Commented Jan 22, 2021 at 1:06
  • 1
    \$\begingroup\$ @JimmyHu Thanks for corrections! Checking it on my PC, made ToDelimitedString public. Looks like it works. \$\endgroup\$
    – aepot
    Commented Jan 22, 2021 at 7:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.