Add Height and Width functions to Rect

[Brett208]

• Add basic unit tests

I am looking at some use cases in the future that could use Width and Height on Rect.

I'm not sure the unit tests are very good. Maybe worth spending some time on them here. I'm also happy just getting a file started for unit testing Rect.

-Brett

[DanRStevens]

I see, yes, the tests seem not so useful. They're just duplicating the implementation. They also seem to have a lot of setup boilerplate which I don't think is needed or really justified here.

I think the tests need to focus more on what you actually want to do with the objects. I would probably construct a couple of Rects inline, immediately call the methods on the temporaries, and validate the return matches an expected hardcoded constant. I'd suggest two such lines for each method. One using the origin (0, 0) as the first point, making the results very easy to see and verify, and then another one with a non-origin point just to make sure the first point is actually used in the calculation.

---

A bit off topic, but might be good practice: I might also suggest extending Rect with other convenience methods that might be easy to validate, or support testing. You can create a Rect from 4 coordinates, but you might also create a Rect from 2 Points. Or you might create a Rect from 1 Point and a width and height, or 1 Point and 1 Vector. That last part would feed well into the current test for width and height. You might support checking for equality of Rects, or containment, or overlap. You might support testing if a Point falls within a Rect. You might also support some kind of bounding box method that returns a Rect which encompasses both/all inputs.

When you have a better sense of what you might want to use an object for, it becomes a little easier to design good tests. It's not just about calling functions to exercise the code, but calling it in ways that supports some objective.

[DanRStevens]

I realize I was being a bit vague and high level there. Let me add an example:

TEST(Rect, CanComputeWidthAndHeight) {
    EXPECT_EQ(640, Rect{0, 0, 640, 480}.Width());
    EXPECT_EQ(480, Rect{0, 0, 640, 480}.Height());

    EXPECT_EQ(2, Rect{10, 10, 12, 15}.Width());
    EXPECT_EQ(5, Rect{10, 10, 12, 15}.Height());
}

Note that construction is only really tested implicitly. I don't think that needs a lot of focus, particularly not for a simple struct which simply has its fields initialized.

[Brett208]

@DanRStevens, I would like to include some convenience constructors with Rect, but then it would not be a POD anymore right? I think adding a constructor means we can no longer do Rect{0, 0, 640, 480}.

An alternative might be:

static Rect Create(const Point32& point1, const Point32& point2);
static Rect Create(const Point32& point, int32_t width, int32_t height);

This would preserve POD and allow creating a Rect with the shorthand. Although I assume this isn't as efficient as creating an actual constructor.

Thanks, Brett

[DanRStevens]

Side note: Why is std::is_pod deprecated?. The concept of POD has been split into trivial and standard layout, where the old definition of POD included both sets of properties. Usually only one set of properties is actually needed in most contexts.

What are aggregates and PODs and how/why are they special?. POD types are a subset of aggregate types.

You are correct in that having a user defined constructor means it would no longer be an aggregate, and so brace initialization would no longer work.

---

I not sure if there is a difference in efficiency between brace initialization or calling a constructor, and if there is, it may not be very significant for most cases. I wouldn't be surprised if brace initialization code appeared identical to an inline constructor call at the assembly level. There is probably a bit of efficiency loss for a non-inline constructor.

If you feel you need constructors, go ahead and create them. Initialization would remain similar, except you'd need to use parenthesis Rect(0, 0, 640, 480) rather than braces Rect{0, 0, 640, 480}. Alternatively, you could use a factory method instead of a constructor, as you've suggested. I'm afraid I don't have any guidelines for when you're want to use a factory method versus using a constructor, or when you should use brace syntax versus parenthesis. My experience with the new brace syntax is limited. Do whatever feels right, and we can sort it out in the end once we know better. :wink:

[Brett208]

Thanks for the feedback. It would make more sense to me to avoid the factory methods then preserve the brace initialization. I may just close for now and come back to later as needed.

-Brett