Skip to content

Conversation

@glen3b
Copy link
Contributor

@glen3b glen3b commented Nov 14, 2016

Currently, the core types Point and Size do not have equality or operators implemented. This PR fixes that. This overrides equality, inequality, addition, subtraction, and unary negation on Point, and all of those except unary negation on Size. The Equals and GetHashCode methods are also implemented, with hash codes using the method recommended by Jon Skeet. These changes will allow cleaner code, such as some of the equality checks in PR #1.

While reviewing this code, I noticed that Size will happily accept negative values for Width and Height. Is this my design? If so, it is not commented, and it should be because it seems contrary to intuition. If not, it should probably be rewritten as a property that validates positive values for both properties. Any thoughts on this?

Copy link
Contributor

@CoolBots CoolBots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contributions!


public override int GetHashCode()
{
// Uses http://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for referencing the source SO post, great read!

{
return new Point(point.X / scalar, point.Y / scalar);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding/subtracting points is not really meaningful mathematically, but I understand we're treating these as Vectors essentially. If we later find a need for a Vector class, these methods should instead be implemented there. At the moment, it's convenient to have them though.

@CoolBots CoolBots merged commit b710f59 into GreatMindsRobotics:master Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants