Problem
I know that there is a generic data structure System.Array could be used to create customized type array. I want to focus on the two dimensional data structure that something like plane with concatenate methods. The following code is my implementation of a generic two dimensional data structure Plane<T>.
Is there any possible improvement of this code?
public class Plane<T>
{
private Array PlaneArray;
public Plane()
{
this.PlaneArray = Array.CreateInstance(typeof(T), 1, 1);
}
public Plane(Plane<T> ObjectInput)
{
this.PlaneArray = ObjectInput.PlaneArray;
}
public Plane(uint SizeXInput, uint SizeYInput)
{
if (SizeXInput < 0 || SizeYInput < 0)
{
this.PlaneArray = Array.CreateInstance(typeof(T), 1, 1);
}
else
{
this.PlaneArray = Array.CreateInstance(typeof(T), SizeXInput + 1, SizeYInput + 1);
}
}
public Plane<T> ConcatenateHorizontal(Plane<T> PlaneInput)
{
if (this.GetSizeY().Equals(PlaneInput.GetSizeY()).Equals(false))
{
return default;
}
var ReturnObject = new Plane<T>(this.GetSizeX() + PlaneInput.GetSizeX(), this.GetSizeY());
Parallel.For(0, this.GetSizeY(), (LoopNumberY, stateY) =>
{
Parallel.For(0, this.GetSizeX(), (LoopNumberX, stateX) =>
{
ReturnObject.SetElement(this.GetElement((uint)LoopNumberX, (uint)LoopNumberY), (uint)LoopNumberX, (uint)LoopNumberY);
});
});
Parallel.For(0, PlaneInput.GetSizeY(), (LoopNumberY, stateY) =>
{
Parallel.For(0, PlaneInput.GetSizeX(), (LoopNumberX, stateX) =>
{
ReturnObject.SetElement(PlaneInput.GetElement((uint)LoopNumberX, (uint)LoopNumberY), (uint)(LoopNumberX + this.GetSizeX()), (uint)LoopNumberY);
});
});
return ReturnObject;
}
public Plane<T> ConcatenateHorizontalInverse(Plane<T> PlaneInput)
{
return PlaneInput.ConcatenateHorizontal(this);
}
public Plane<T> ConcatenateVertical(Plane<T> PlaneInput)
{
if (this.GetSizeX().Equals(PlaneInput.GetSizeX()).Equals(false))
{
return new Plane<T>();
}
var ReturnObject = new Plane<T>(this.GetSizeX(), this.GetSizeY() + PlaneInput.GetSizeY());
Parallel.For(0, this.GetSizeY(), (LoopNumberY, stateY) =>
{
Parallel.For(0, this.GetSizeX(), (LoopNumberX, stateX) =>
{
ReturnObject.SetElement(this.GetElement((uint)LoopNumberX, (uint)LoopNumberY), (uint)LoopNumberX, (uint)LoopNumberY);
});
});
Parallel.For(0, PlaneInput.GetSizeY(), (LoopNumberY, stateY) =>
{
Parallel.For(0, PlaneInput.GetSizeX(), (LoopNumberX, stateX) =>
{
ReturnObject.SetElement(PlaneInput.GetElement((uint)LoopNumberX, (uint)LoopNumberY), (uint)LoopNumberX, (uint)(LoopNumberY + this.GetSizeY()));
});
});
return ReturnObject;
}
public Plane<T> ConcatenateVerticalInverse(Plane<T> BlockStructureInput)
{
return BlockStructureInput.ConcatenateVertical(this);
}
public T GetElement(uint LocationX, uint LocationY)
{
if (LocationX <= this.PlaneArray.GetUpperBound(0) &&
LocationX >= this.PlaneArray.GetLowerBound(0) &&
LocationY <= this.PlaneArray.GetUpperBound(1) &&
LocationY >= this.PlaneArray.GetLowerBound(1))
{
return (T)this.PlaneArray.GetValue(LocationX, LocationY);
}
Console.WriteLine("Index invalid");
return default(T);
}
public uint GetSizeX()
{
return (uint)(this.PlaneArray.GetUpperBound(0) - this.PlaneArray.GetLowerBound(0));
}
public uint GetSizeY()
{
return (uint)(this.PlaneArray.GetUpperBound(1) - this.PlaneArray.GetLowerBound(1));
}
public Tuple<bool, Plane<T>> SetElement(T NewElement, uint LocationX, uint LocationY)
{
if (LocationX <= this.PlaneArray.GetUpperBound(0) &&
LocationX >= this.PlaneArray.GetLowerBound(0) &&
LocationY <= this.PlaneArray.GetUpperBound(1) &&
LocationY >= this.PlaneArray.GetLowerBound(1))
{
this.PlaneArray.SetValue(NewElement, LocationX, LocationY);
return new Tuple<bool, Plane<T>>(true, this);
}
else
{
return new Tuple<bool, Plane<T>>(false, this); ;
}
}
public override string ToString()
{
string ReturnString = "";
for (uint LoopNumberY = 0; LoopNumberY < this.GetSizeY(); LoopNumberY++)
{
for (uint LoopNumberX = 0; LoopNumberX < this.GetSizeX(); LoopNumberX++)
{
ReturnString = ReturnString + this.GetElement(LoopNumberX, LoopNumberY).ToString() + "t");
}
ReturnString = ReturnString + System.Environment.NewLine;
}
return ReturnString.ToString();
}
}
The usage of this Plane<T> class:
Plane<string> plane = new Plane<string>(5, 5);
for (uint LoopNumberY = 0; LoopNumberY < plane.GetSizeY(); LoopNumberY++)
{
for (uint LoopNumberX = 0; LoopNumberX < plane.GetSizeX(); LoopNumberX++)
{
plane.SetElement("(" + LoopNumberX.ToString() + ", " + LoopNumberY.ToString() + ")", LoopNumberX, LoopNumberY);
}
}
Console.WriteLine(plane.ToString());
var ConcatenateHRetult = plane.ConcatenateHorizontal(plane);
Console.WriteLine(ConcatenateHRetult.ToString());
var ConcatenateVRetult = plane.ConcatenateVertical(plane);
Console.WriteLine(ConcatenateVRetult.ToString());
Oct 16, 2020 Update
In order to test this Plane class, the customized method Equals is needed. Here’s my implementation. First, the size comparison is performed, and the next step is checking the content equivalent.
public override bool Equals(object obj)
{
var objForTesting = obj as Plane<T>;
if (this.GetSizeX().Equals(objForTesting.GetSizeX()).Equals(false) ||
this.GetSizeY().Equals(objForTesting.GetSizeY()).Equals(false)
) // If size is different
{
return false;
}
// Content comparison
for (uint y = 0; y < this.GetSizeY(); y++)
{
for (uint x = 0; x < this.GetSizeX(); x++)
{
if (this.GetElement(x, y).Equals(objForTesting.GetElement(x, y)).Equals(false))
{
return false;
}
}
}
return true;
}
Solution
First I would like to re-iterate the points made by @cliesens regarding naming and avoid uint.
Keep in mind the most number of elements you may have in an array is int.MaxValue. Theoretically, with your use of unit, someone could try to create an array that is 3 billion wide by 3 billion deep, which would throw exceptions. And your code doesn’t handle exceptions very well.
UPDATED: Was it intentional that your constructor accepting a Plane copies the source plane by reference? That would mean that anyone changing something in the copy would also be changing it in the original and vice versa. I personally would think this should be a cloned copy, instead of a reference copy.
The ToString() is a bad idea, not because of the lack of StringBuilder but because it could be a very long process. Let’s say I had a plane that is 40K across and 50K tall, for 2 billion elements. Then I must wait a really long time for ToString() to fully render a result. Since the debugger and other things use ToString() to quickly display information, you should have ToString() be as fast as it can be.
Thus, I would suggest changing it, and moving your current logic into its own method. I do find it odd that you choose to display the array by Y and then X (or by Height first then Width) since most people working with a 2D geometric plane tend to think in terms of X first, then Y. Perhaps you chose to do this so that as it prints down a page it would almost textually mirror the plane rotated around the X-axis. If so, this would be predicated upon the elements in the X dimension printing neatly across 1 row in the Console window.
I would suggest you may want to have 2 different methods so that one has a choice of the display order.
I would also suggest there could be properties in your class for Width and Height, which seem more natural to use rather than GetSizeX() or GetSizeY().
You employ System.Array even though you have a generic class. All of your calls to CreateInstance create a typically 0-indexed array, so calling GetUpperBound(0) - GetLowerBound(0) + 1 could be replaced with GetLength(0).
Here is my take on a partial design:
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);
}
// SNIPPED
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
} // namespace
That’s about all I can do since I must get back to my daytime job. To summarize:
- Consider using properties to your advantage
- Employ suggested C# naming guidelines
- Use short, simple naming for loop variables
- Avoid
unitfor array indexing - Have a Width and Height property
- Consider using a generic array
- ToString should be short, sweet and FAST
- Offer a choice to display (X by Y) or (Y by X)
Given that an array can hold int.MaxValue elements, then your biggest square plane would be 46_340 X 46_340. If you wanted to offer HUGE planes, then you would need to change the guts of the class to support an array of arrays.
Other things to consider:
- Should you implement IEquatable or IComparable?
- Do you want any constraints, such as struct, on T?
UPDATE #2 : C# INDEXERS
I find having GetElement and SetElement methods cumbersome. I prefer to interact with the object more like an array, and that’s where C# Indexers come in handy. Here is a way you can get rid of both GetElement and SetElement, while maintaining the same functionality:
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;
}
}
And here is a quick example of how to use it:
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++)
{
//NOTICE how I use plane like a 2D array
plane[x, y] = ((x + 1) * 100) + y;
}
Console.WriteLine("ToString() Example: " + plane.ToString());
Console.WriteLine("nTAB DELIMITED BY X THEN Y");
Console.WriteLine(plane.ToDelimitedStringByWidthThenHeight());
Console.WriteLine("nSEMI-COLON DELIMITED BY Y THEN X");
Console.WriteLine(plane.ToDelimitedStringByHeightThenWidth(" ; "));
CONSOLE OUTPUT
ToString() Example: PlaneV2<Int32>[3, 5]
TAB DELIMITED BY X THEN Y
100 101 102 103 104
200 201 202 203 204
300 301 302 303 304
SEMI-COLON DELIMITED BY Y THEN X
100 ; 200 ; 300
101 ; 201 ; 301
102 ; 202 ; 302
103 ; 203 ; 303
104 ; 204 ; 304
Welcome to Code Review!
I don’t have much time to write a full review right now, but I’ll give you a concise list of things you could do to improve your code (in my opinion).
Naming
Use camelCase instead of PascalCase for local variables, class variables & method arguments. In addition to making your code a lot more readable (I don’t have to wonder, “is this a variable? a method? a static class?”), it also ensures other people who are familiar with C# will be able to understand and work with your code, as this is the convention followed by most (if not all) C# programmers.
I’m also not a very big fan of super long names for the for loop iterating variable. I generally stick to i, j and sometimes k, or use other descriptive names when I’m not simply iterating over indices. But to me, LoopNumberX is really way too long and actually makes the code harder to read. What does LoopNumberX even mean? Again, this is a personal preference, but I don’t think there’s many people who would argue LoopNumberX is a good name. If you know what a for loop is, then whenever you see i or j, you’ll know exactly what is going on.
Properties
You could make your code even more maintainable & easy to understand by using properties. You should use PascalCase when naming properties. Here are some examples of using properties in various ways:
private int x;
public int X
{
public get
{
return x;
}
public set
{
x = value;
}
}
Which you can shorten to :
private int x;
public int X
{
public get => x;
public set => x = value;
}
As there is only a single statement is the getter/setter. You can go even further and remove the private field entirely like this :
public int X { get; private set; }
This is called an auto-property. This example also shows how you can give the property, its setter and getter different access modifiers. You can also add logic inside the getter/setter like this :
private int x;
public int X
{
public get { return x; }
public set
{
if (x < 0) {
throw new Exception("X should be larger than 0!");
}
x = value;
}
}
Properties are great, use them whenever you can (though they shouldn’t replace all your methods of course, only when appropriate)!
uint
I’d recommend against using uint everywhere as you do, even though it may seem like a good idea. If you’d like more information on this subject, I recommend taking a quick look at https://stackoverflow.com/questions/2013116/should-i-use-uint-in-c-sharp-for-values-that-cant-be-negative.
This:
Console.WriteLine("Index invalid");
return default(T);
is dangerous. Having out-of-bounds indices is dangerous enough that you should probably just raise an exception here, rather than returning a default that will probably be wrong from the perspective of the caller. If this is a feature that you’re really going to use, wrap it in a second function (TryGetElement) that follows the same pattern as other methods in C#: have an output parameter that might be set, and return a boolean that is only true if the call succeeded.