-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Line2D.DistanceTo(Point2D p) #238
base: master
Are you sure you want to change the base?
Conversation
new object[] { "+1,-1", "+1,+1", "3,0", 2.0 }, | ||
}; | ||
|
||
[TestCaseSource(nameof(DistanceTestCases))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous structure with TestCase
was much better. My suggestion with the swapped scenarios was meant to be handled in the TestCase
definition, not as a separate test function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
Could you tell me why using TestCode
is better than using TestCodeSource
for this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the whole new object[]
a bit overkill, that's it. I guess it's more a matter of personal taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
it's more a matter of personal taste.
Could you show the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for what exactly? I prefer compile-time checks and try to avoid object
as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for telling the reasons.
Done in the commit cd80cf1.
I changed the test-code as you required. I'm not sure that the revised test-code helps the reader understand at a glance that parameters are kept intact except its order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This is not what I meant, we can augment the existing tests with a reversed line
var line = new Line2D(Point2D.Parse(p1s), Point2D.Parse(p2s));
// existing test
var reversedLine = new Line2D(Point2D.Parse(p2s), Point2D.Parse(p1s));
// distance test with reversed line
This way you keep the TestCase
cases at a minimum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for showing the example closely.
It is not recommended to test 2 things in a single unit-test.
Or do you want to say that these 2 tests are equivalent because of the symmetry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not recommended to test 2 things in a single unit-test.
Fully agree
Or do you want to say that these 2 tests are equivalent because of the symmetry?
Basically yes
I implemented
Line2D.DistanceTo(Point2D p)
which calculates the distance fromthis
line to the given pointp
.This method should always return the value of 0 or positive.
It will be nice if we can discuss whether the testcases is sufficient.