Skip to content

Conversation

@yoc8
Copy link

@yoc8 yoc8 commented Jul 28, 2024

No description provided.

Copy link
Owner

@gemtechd gemtechd left a comment

Choose a reason for hiding this comment

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

Add more checks in your code
then you'll have to write more tests and use better mocks
in the tests: give each test a description, and use the it test function and not test

throw new Error('the constructor should get two arguments of "Point"')
}
if ((typeof (n) !== "undefined" && typeof (n) !== "number") || (typeof (slope) !== 'number' && typeof (slope) !== "undefined")) {
throw new Error('the n and slope in constractor should get undefined or number')
Copy link
Owner

Choose a reason for hiding this comment

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

Which one of the parameter threw an error??

}

calculateSlope() {
calculateSlope = () => {//שיפוע
Copy link
Owner

Choose a reason for hiding this comment

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

change it back to calculateSlope() { } from calculateSlope = ()=>{}


calculateSlope() {
calculateSlope = () => {//שיפוע
this.slope = (this.point1.y - this.point2.y) / (this.point1.x - this.point2.x)
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if point1.x - point2.x === 0??


calculateNOfLineFunction() {
calculateNOfLineFunction = () => {//מרחק
this.n = this.point1.y - this.slope * this.point1.x
Copy link
Owner

Choose a reason for hiding this comment

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

was the slope already calculated?

throw new Error('the function should get two arguments of "Point"')
}
if ((!(point1 instanceof Point)) || (!(point2 instanceof Point))) {
throw new Error('the function should get two arguments of "Point"')
Copy link
Owner

Choose a reason for hiding this comment

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

Which parameter threw the error and why?

return mPoint;
});

const result2 = line1.getPointOnYAsis();
Copy link
Owner

Choose a reason for hiding this comment

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

this test is a bit weird: you description is not what you really do

Copy link
Owner

Choose a reason for hiding this comment

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

  1. give a good description for each test
  2. use the it test function instead of test

const point1 = new Point({ x: 1, y: 1 })
const point2 = new Point({ x: 2, y: 2 })
const line1 = new Line({ point1, point2, n: 2, slope: 2 })

Copy link
Owner

Choose a reason for hiding this comment

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

don't declare parameters in the head of the module, declare each one inside the test


describe('CALCULATE_DISTANCE', () => {
test('', () => {
expect(geometry.calculateDistance(point1, point2)).toBe(1.4142135623730951)
Copy link
Owner

Choose a reason for hiding this comment

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

  1. write a description
  2. get a nice number

Copy link
Owner

Choose a reason for hiding this comment

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

change your code, and the tests will need more mocks

@yoc8
Copy link
Author

yoc8 commented Jul 29, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants