3
\$\begingroup\$

This is a follow-up question for Generic Two Dimensional Data Plane with Manipulation Methods For C#. Thanks to cliesens's and Rick Davin's detailed answer. Moreover, the mentioned PlaneV2 example is very clear to let me realize how to improve my code. Based on Rick Davin's PlaneV2 example, I am trying to implement a SubPlane method which can extract a specific region data block by given parameters. The experimental code is as follows.

public PlaneV2<T> SubPlane(int locationX, int locationY, int newWidth, int newHeight)
{
    if (this.Grid == null)
    {
        return null;
    }
    PlaneV2<T> outputPlaneV2 = new PlaneV2<T>(newWidth, newHeight);

    for (int y = 0; y < newHeight; y++)
    {
        for (int x = 0; x < newWidth; x++)
        {
            outputPlaneV2[x, y] = this.Grid[locationX + x, locationY + y];
        }
    }
    return outputPlaneV2;
}

As you can see, there are four parameters in this SubPlane method, The first and second parameter represents the top-left location in the origin plane, and the third and forth parameter represents the size of the extracted sub-plane. The usage of this SubPlane method is as below.

var width = 3;
var height = 5;
var plane = new PlaneV2<int>(width, height);

for (var x = 0; x < width; x++)
    for (var y = 0; y < height; y++)
        plane[x, y] = ((x + 1) * 100) + y;

var newSubPlane = plane.SubPlane(1, 1, 2, 2);           //  For example

Console.WriteLine("ToString() Example: " + newSubPlane.ToString());

Console.WriteLine("\nTAB DELIMITED BY X THEN Y");
Console.WriteLine(plane.ToDelimitedStringByWidthThenHeight());
Console.WriteLine(newSubPlane.ToDelimitedStringByWidthThenHeight());

Console.WriteLine("\nSEMI-COLON DELIMITED BY Y THEN X");
Console.WriteLine(plane.ToDelimitedStringByHeightThenWidth(" ; "));
Console.WriteLine(newSubPlane.ToDelimitedStringByHeightThenWidth(" ; "));

The whole PlaneV2<T> class with SubPlane method implementation:

public class PlaneV2<T>
{
    public int Width { get; } = 0;
    public int Height { get; } = 0;
    private T[,] Grid = null;
    public int Length => Width * Height;

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

    public PlaneV2(int width, int height)
    {
        Width = Math.Max(width, 0);
        Height = Math.Max(height, 0);
        Grid = new T[width, height];
    }

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

    public PlaneV2(T[,] sourceGrid)
    {
        if (sourceGrid == null)
        {
            return;
        }

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

    public T this[int x, int y]
    {
        get
        {
            if (x < 0 || x >= Width)
            {
                throw new ArgumentOutOfRangeException(nameof(x));
            }
            if (y < 0 || y >= Height)
            {
                throw new ArgumentOutOfRangeException(nameof(y));
            }
            return Grid[x, y];
        }
        set
        {
            if (x < 0 || x >= Width)
            {
                throw new ArgumentOutOfRangeException(nameof(x));
            }
            if (y < 0 || y >= Height)
            {
                throw new ArgumentOutOfRangeException(nameof(y));
            }
            Grid[x, y] = value;
        }
    }

    public PlaneV2<T> SubPlane(int locationX, int locationY, int newWidth, int newHeight)
    {
        if (this.Grid == null)
        {
            return null;
        }
        PlaneV2<T> outputPlaneV2 = new PlaneV2<T>(newWidth, newHeight);

        for (int y = 0; y < newHeight; y++)
        {
            for (int x = 0; x < newWidth; x++)
            {
                outputPlaneV2[x, y] = this.Grid[locationX + x, locationY + y];
            }
        }
        return outputPlaneV2;
    }

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

    public string ToDelimitedStringByHeightThenWidth(string separator = "\t")
    {
        StringBuilder lines = new StringBuilder();
        for (int y = 0; y < Height; y++)
        {
            StringBuilder columns = new StringBuilder();
            string columnDelimiter = "";
            for (int x = 0; x < Width; x++)
            {
                columns.Append(columnDelimiter + Grid[x, y].ToString());
                if (columnDelimiter == "")
                {
                    columnDelimiter = separator;
                }
            }
            lines.AppendLine(columns.ToString());
        }
        return lines.ToString();
    }

    public string ToDelimitedStringByWidthThenHeight(string separator = "\t")
    {
        StringBuilder lines = new StringBuilder();
        for (int x = 0; x < Width; x++)
        {
            StringBuilder columns = new StringBuilder();
            string columnDelimiter = "";
            for (int y = 0; y < Height; y++)
            {
                columns.Append(columnDelimiter + Grid[x, y].ToString());
                if (columnDelimiter == "")
                {
                    columnDelimiter = separator;
                }
            }
            lines.AppendLine(columns.ToString());
        }
        return lines.ToString();
    }

} // class

All suggestions are welcome.

  • Which question it is a follow-up to?

    Generic Two Dimensional Data Plane with Manipulation Methods For C#

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

    The previous question focused on the concatenation methods, and the main idea here is to implement a SubPlane method which can extract a specific region data block by given parameters.

  • Why a new review is being asked for?

    Although it seems that the above version of this SubPlane method works well, I am not sure is it efficient with copying data element-by element using for loop. I am not sure how to construct the output sub-plane with Array.Copy in this situation. If there is any better idea, please let me know.

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

A few points :

  • The default value of int is zero, so public int Width { get; } = 0; is unnecessary.

  • Width = Math.Max(width, 0); what is the reason of doing this ? isn't going to give you the same as Width = width; or is there is some cases where the values would differ ?

  • In the third constructor if (sourceGrid == null) { return; }, not recommended, either throw ArguementNullException or you could initiate the default instructor would be better.

     public T this[int x, int y]
     {
         get
         {
             if (x < 0 || x >= Width)
             {
                 throw new ArgumentOutOfRangeException(nameof(x));
             }
             if (y < 0 || y >= Height)
             {
                 throw new ArgumentOutOfRangeException(nameof(y));
             }
             return Grid[x, y];
         }
         set
         {
             if (x < 0 || x >= Width)
             {
                 throw new ArgumentOutOfRangeException(nameof(x));
             }
             if (y < 0 || y >= Height)
             {
                 throw new ArgumentOutOfRangeException(nameof(y));
             }
             Grid[x, y] = value;
         }
     }
    

is equal to this :

public T this[int x, int y]  { get; set; }

Unless you implement a custom validation, your current indexer will be redundant, because it replicates the indexer default behavior. Mostly, you need to implement your requirements within the setter, but the getter in most cases doesn't need more than default behavior. As you're required to handle what's going inside your class (setter), but not what goes out of it (getter).

  • Combine : ToDelimitedStringByHeightThenWidth and ToDelimitedStringByWidthThenHeight code into one private method and just recall it from other methods.

Example :

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

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

private string ToDelimitedString(int outterCounter, int innerCounter, string separator)
{
    var lines = new StringBuilder();
    var columns = new StringBuilder();
    var columnDelimiter = string.IsNullOrWhiteSpace(separator) ? "" : separator;
    
    for (int x = 0; x < outterCounter; x++)
    {
        for (int y = 0; y < innerCounter; y++)
        {
            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();
}

For SubPlane, you should not use the regular for loop to copy arrays unless if you're sure that it won't be a big arrays (large arrays = 2000+ elements). The reason behind that is the more elements that array has, the more performance you lose when copying using the for loop. So, to gain more performance, you need to use Array.Copy for large arrays, which is a native function that would copy the array with bypassing some of CLR checks. This would eventually add some extra boost to the overall performance.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ The part about the getter is not correct. Assume setting I first set plane[1, 1] = 1 and want to read (assuming T is int) int value = plane[-1, 1] \$\endgroup\$
    – Heslacher
    Nov 4, 2020 at 12:17
  • \$\begingroup\$ @Heslacher by default, it would throw an index out of range exception, which I think it's redundant. \$\endgroup\$
    – iSR5
    Nov 4, 2020 at 12:27
  • \$\begingroup\$ Well, my bad you are right ;-) \$\endgroup\$
    – Heslacher
    Nov 4, 2020 at 12:35
  • \$\begingroup\$ @Heslacher nope, I'm the one who needs to apology, I read my lines, and it felt misleading, so I updated that line to be more clear. Accept my apologies again. \$\endgroup\$
    – iSR5
    Nov 4, 2020 at 12:53
  • \$\begingroup\$ no problem, no need to apologize \$\endgroup\$
    – Heslacher
    Nov 4, 2020 at 12:58
1
\$\begingroup\$

Only focusing on ToDelimitedStringByHeightThenWidth() and ToDelimitedStringByWidthThenHeight()

By extracting the methods IEnumerable<string> TopDown(int y) and IEnumerable<string> TopDown(int y) like so:

private IEnumerable<string> TopDown(int y)
{
    for (int x = 0; x < Width; x++)
    {
        yield return Grid[x, y].ToString();
    }
}  
private IEnumerable<string> LeftRight(int x)
{
    for (int y = 0; y < Height; y++)
    {
        yield return Grid[x, y].ToString();
    }
}  

you can take advantage of the string.Join(separator, IEnumerable<string>) and remove the StringBuilder columns and the if inside the most inner loops, leading to:

public string ToDelimitedStringByHeightThenWidth(string separator = "\t")
{
    StringBuilder lines = new StringBuilder();
    for (int y = 0; y < Height; y++)
    {
        lines.AppendLine(string.Join(separator, TopDown(y)));
    }
    return lines.ToString();
}  
public string ToDelimitedStringByWidthThenHeight(string separator = "\t")
{
    StringBuilder lines = new StringBuilder();
    for (int x = 0; x < Width; x++)
    {
        lines.AppendLine(string.Join(separator, LeftRight(x)));
    }
    return lines.ToString();
}
\$\endgroup\$

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.