3
\$\begingroup\$

I'm trying to create a private graphics library. Even tho I'm doing so for fun, I'd like to have some feedback about my coding. For instance, what do you guys think of the following class (Rectangle)? It does have some dependencies on other classes that I'm not including here just to save space. If you guys think they're relevant, lemme know and I'll edit the answer to include'em.

Rectangle's dependencies are:

Point: A 2d point with integer coordinates.
Size: Two integers whos values can only be positive (maybe I should change it to uint?)
RectangleF: A 2d rectangle with float coordinates.

My most important questions are:

  1. If I don't allow negative Widths nor Heights, should their types be uint? I'm using int because most of the "size" variables in .net are int (instead of uint). Ie: System.Collections.Generic.List's Count it a int, even tho it can never be negative.
  2. Is it a bad design for my rectangle to implement IEnumerable? Is it counter intuitive?

Oh, and I'm using public readonly fields (instead of get only properties) to make sure that even I can't change their values by accident.

namespace Trauer.Graphics
{
    /// <summary>
    /// Immutable struct to replace mutable struct System.Drawing.Rectangle.
    /// </summary>
    public struct Rectangle : IEquatable<Rectangle>, IEnumerable<Point>
    {
        public readonly int X;
        public readonly int Y;
        public readonly int Width;
        public readonly int Height;

        public int Left => X;
        public int Top => Y;
        public int Right => X + Width;
        public int Bottom => Y + Height;

        public Point Location => new Point(X, Y);
        public Size Size => new Size(Width, Height);

        public static Rectangle Empty;

        public Rectangle(int x, int y, int width, int height)
        {
            if (width < 0)
                throw new ArgumentOutOfRangeException(nameof(width) + " must be equal to or greater than zero.");
            if (height < 0)
                throw new ArgumentOutOfRangeException(nameof(height) + " must be equal to or greater than zero.");

            X = x;
            Y = y;
            Width = width;
            Height = height;
        }

        public Rectangle(Point location, Size size)
        {
            X = location.X;
            Y = location.Y;
            Width = size.Width;
            Height = size.Height;
        }

        public static Rectangle FromLTRB(int left, int top, int right, int bottom) => new Rectangle(left, top, right - left, bottom - top);

        public override string ToString() => $"{{X={X},Y={Y},Width={Width},Height={Height}}}";

        public override int GetHashCode()
        {
            // Thanks Microsoft ._.
            return unchecked((int)((uint)X ^
                        (((uint)Y << 13) | ((uint)Y >> 19)) ^
                        (((uint)Width << 26) | ((uint)Width >> 6)) ^
                        (((uint)Height << 7) | ((uint)Height >> 25))));
        }

        public override bool Equals(object obj) => obj is Rectangle && Equals((Rectangle)obj);

        public bool Equals(Rectangle other)
        {
            return X == other.X &&
                   Y == other.Y &&
                   Width == other.Width &&
                   Height == other.Height;
        }

        public static bool operator ==(Rectangle left, Rectangle right)
        {
            return (left.X == right.X
                    && left.Y == right.Y
                    && left.Width == right.Width
                    && left.Height == right.Height);
        }

        public static bool operator !=(Rectangle left, Rectangle right) => !(left == right);

        public IEnumerator<Point> GetEnumerator()
        {
            return GetContainedPoints().GetGenericEnumerator();
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetContainedPoints().GetEnumerator();
        }

        public Point[] GetContainedPoints()
        {
            var points = new Point[Width * Height];
            int index = 0;
            for (int y = Y; y < Bottom; y++)
            {
                for (int x = X; x < Right; x++)
                {
                    points[index] = new Point(x, y);
                    index += 1;
                }
            }
            return points;
        }

        public System.Drawing.Rectangle ToSystemRectangle() => new System.Drawing.Rectangle(X, Y, Width, Height);

        public RectangleF ToRectangleF() => new RectangleF(X, Y, Width, Height);

        public bool Contains(int x, int y)
        {
            return x >= X &&
                   x < Right &&
                   y >= Y &&
                   y < Bottom;
        }

        public bool Contains(Point pt) => Contains(pt.X, pt.Y);

        public bool Contains(Rectangle other)
        {
            return other.X >= X &&
                   other.Right <= Right &&
                   other.Y >= Y &&
                   other.Bottom <= Bottom;
        }

        public Rectangle Intersect(Rectangle other)
        {
            int biggestX = Math.Max(X, other.X);
            int smallestRight = Math.Min(Right, other.Right);
            int biggestY = Math.Max(Y, other.Y);
            int smallestBottom = Math.Min(Bottom, other.Bottom);

            if (smallestRight >= biggestX && smallestBottom >= biggestY)
                return new Rectangle(biggestX, biggestY, smallestRight - biggestX, smallestBottom - biggestY);
            else
                return Empty;
        }

        public bool IntersectsWith(Rectangle other)
        {
            return other.X < Right &&
                   X < other.Right &&
                   other.Y < Bottom &&
                   Y < other.Bottom;
        }

        public Rectangle Union(Rectangle other)
        {
            int smallestX = Math.Min(X, other.X);
            int biggestRight = Math.Max(Right, other.Right);
            int smallestY = Math.Min(Y, other.Y);
            int biggestBottom = Math.Max(Bottom, other.Bottom);

            return new Rectangle(smallestX, smallestY, biggestRight - smallestX, biggestBottom - smallestY);
        }
    }
}

Regarding a few questions:

Q: Why are you implementing your own graphics library? A: I started messing around with it when I was trying to understand why the system's Bitmap's Get/Set methods are so slow. Eventually I found myself having quite a lot of fun playing around with "my own library" (even if it wasn't suitable for production, since it was so poorly commented and had almost no unit tests), so I decided to continue playing with it.

Q: Why are you implementing your own Rectangle instead of using the System's one? A: First and foremost, because I don't like the idea of a mutating Rectangle. I'm not against mutable objects. But for "really simple things" like a struct that represent a Rectangle, I feel that changing one of its properties does change it's "essence". So I should create a new one instead. Second because I'm trying to reduce my library dependencies on System.Graphics: It doesn't feel right to create a graphics lib (that doesn't bring anything new to the table) that depends on another one (even bigger), right? So, even tho this is mostly a fun project, I'm trying to reduce my lib's dependencies on System.Graphics. Right now, my Bitmap class has a strong dependence on the System's one to read/write to files. And most of my classes contain a "ToSystemXXX" method. But in the future, if I implement a legit way to read / write a Bitmap to a files, I could just remove such methods and be "free" of the System.Graphics.

Q: Why does your rectangle implement IEnumerable? A: Well... That's one of the reasons I came here: to get feedback on that. My reasoning is that it makes other classes a little bit leaner. Bitmap, for instance, doesn't need to implement a "SetPixels(Rectangle, Color)" method, since it already implements a "SetPixels(IEnumerable, Color)". But I'm getting a "this is too pythonic for C# lib" feel. I'm not sure if this convenience justifies it's implementation, since it's hurting... I don't know... The "semantic" of the Rectangle.

Q: Did you steal Mycrosoft's GetHashCode implementation? A: Yes ._. as you can see in my GetHashCode comment.

Edit: I decided that my "shapes" won't implement IEnumerable. Even tho it's quite practical (and fun) to play with IEnumerables, I think it's not a good design choice, in this case. Mostly because it's not immediately clear which points you'll get when iterating of a shape that implements IEnumerable. In this case [rectangle], if you iterate over it, do you expect to get the points of its edge or the ones inside it? Or both? So yeah... I'll refactor my code. Thanks for the feedback guys and girls!

\$\endgroup\$
  • \$\begingroup\$ This is a nice QA list you've added and the answers seem to be resonable - although they mean a lot of work - but - this also means you will probably have more questions so I'm looking forward to reviewing them too - should I have something to comment on ;-) \$\endgroup\$ – t3chb0t Feb 19 '17 at 16:23
  • \$\begingroup\$ By all means, please do. \$\endgroup\$ – Trauer Feb 19 '17 at 16:34
  • \$\begingroup\$ If you're trying hard to decouple and eliminate dependencies, why not go a step further and get rid of Point and Size? \$\endgroup\$ – Svek Feb 20 '17 at 6:52
  • \$\begingroup\$ Oh, and I'm using public readonly fields (instead of get only properties) to make sure that even I can't change their values by accident: "get only properties" can't be changed as well. \$\endgroup\$ – JanDotNet Feb 20 '17 at 8:09
2
\$\begingroup\$

Oh, and I'm using public readonly fields (instead of get only properties) to make sure that even I can't change their values by accident.

This is a good thing, but you might step into the Empty bear trap. Your Empty field is public static which makes it possible to do something like

Rectangle a = new Rectangle(1, 2, 0, 0);
Rectangle.Empty = a;

Console.WriteLine(Rectangle.Empty.Left);  

which produces the output 1.

Changing

public static Rectangle Empty;  

to

public static readonly Rectangle Empty;  

will remove the trap.

\$\endgroup\$
  • \$\begingroup\$ This really is a bug! Thanks! \$\endgroup\$ – Trauer Feb 20 '17 at 14:27
3
\$\begingroup\$

Rectangle as IEnumerable

Is it a bad design for my rectangle to implement IEnumerable? Is it counter intuitive?

I find a Rectangle implementing the IEnumerable interface is weird and counter intuitive unless you have a good reason for implementing it. Unfortunatelly you didn't give any examples how to use it. Even the FromLTRB does not rely on this interface although it could be a good candidate but then you would need to store the coordinates in an array... or enumerate the values and check the indicies (perhaps with a switch). A lot of work.

Other then this I don't know what to expect from it and why would I need it.

Anyways, I think rectangle extensions would be better for this purpose because then you could write more then one that has a fixed order. In the name of the extension you could put the order of the coordinates e.g.:

public static IEnumerable<Point> AsEnumerableLTRB(this Rectangle rect) { ... }

or

public static IEnumerable<Point> AsEnumerableTRBL(this Rectangle rect) { ... }

alternatively only one extension with a PointOrder option.

implicit/explicit operators

public System.Drawing.Rectangle ToSystemRectangle() => new System.Drawing.Rectangle(X, Y, Width, Height);

public RectangleF ToRectangleF() => new RectangleF(X, Y, Width, Height);

These could do better as implicit or explicit operators.


I also wonder why you need a new rectangle? What can this one do better then the original one? The only difference I see is immutability but unless your graphics library can work directly with this rectangle I think the additional overhead of translating this type into the native one is an overkill and not worth it.


Int32 vs UInt32

If I don't allow negative Widths nor Heights, should their types be uint? I'm using int because most of the "size" variables in .net are int (instead of uint). Ie: System.Collections.Generic.List's Count it a int, even tho it can never be negative.

I think this question on Stack Overflow about [using uint vs int] can give you a good answer why you should use int and not uint. In short:

UInt32 is not CLS Compliant, which means that it is wholly inappropriate for use in public APIs. If you're going to be using uint in your private API, this will mean doing conversions to other types - and it's typically easier and safer to just keep the type the same.

(you should read the other answers on SO as well).

\$\endgroup\$
  • \$\begingroup\$ Indeed, my library can work with this one (and doesn't work with this system's one). Besides immutability it has a smaller memory foot print. Implementing IEnumerable is kinda weird... It does have it's uses... But it's far too pythonic for a C# project, I think... \$\endgroup\$ – Trauer Feb 19 '17 at 4:55
  • \$\begingroup\$ @Trauer it has a smaller memory foot print this makes me curious. Could you explain why because when I look at the source code of the system rectangle they use for fields and you use four fields, the only difference is that your fields are readonly, everything else is exactly the same. Sometimes it looks like you have just copy/pasted their code, renamed the variables and made them readonly ;-] I'm also curious why you need it to be enumerable. \$\endgroup\$ – t3chb0t Feb 19 '17 at 5:59
  • \$\begingroup\$ Because I thought that the system's one "bottom" and "right" were stored, like JanDotNet suggested me to do. And I actually did copy Microsoft's GetHashCode implementation. I'll edit the questions to explain why I implement IEnumerable. \$\endgroup\$ – Trauer Feb 19 '17 at 14:31
2
\$\begingroup\$

The Rectangle struct is well implemented IMHO.

Just a few remarks:

  • It is not necessary to implement the == operator again, just use the Equals method.
  • Calculate Right an Bottom once in the constructor to avoid multiple calculations.
  • I would expect, that Right and Bottom are inside the Rectangle.
  • Use properties instead of public fields for X, Y, Width and Height

If I don't allow negative Widths nor Heights, should their types be uint? I'm using int because most of the "size" variables in .net are int (instead of uint). Ie: System.Collections.Generic.List's Count it a int, even tho it can never be negative.

I wouldn't use uint - throwing an ArgumentOutOfRangeException is absolutely OK here

Is it a bad design for my rectangle to implement IEnumerable? Is it counter intuitive?

In my eyes - yes. It is not clear what to get when iterating over an rectangle.

\$\endgroup\$
  • \$\begingroup\$ What about using uint instead of int? \$\endgroup\$ – Trauer Feb 18 '17 at 21:49

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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