-
Notifications
You must be signed in to change notification settings - Fork 41
finish function and t #17
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
base: main
Are you sure you want to change the base?
Conversation
gemtechd
left a comment
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.
Your mocking is not the right way
modules/ecs6-class/line.js
Outdated
| class Line { | ||
| constructor({ point1 = new Point(), point2 = new Point(), n = undefined, slope = undefined }) { | ||
| if (typeof (point1) !== "object" && typeof (point2) !== "object" && typeof (n) !== "number" && typeof (slope) != "number") { | ||
| throw Error('all parametrs isnt number') |
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 don't understand this check
n and slope can be undefined.
modules/ecs6-class/line.js
Outdated
| getPointOnXAsis: Line.prototype.getPointOnXAsis, | ||
| calculateNOfLineFunction: Line.prototype.calculateNOfLineFunction, | ||
| calculateSlope: Line.prototype.calculateSlope, | ||
| } No newline at end of file |
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.
This all exports here is unnecessary.
return it to the origin state
modules/ecs6-class/point.js
Outdated
| } | ||
|
|
||
| module.exports = Point No newline at end of file | ||
| module.exports = { Point, moveVertical: Point.prototype.moveVertical, moveHorizontal: Point.prototype.moveHorizontal } No newline at end of file |
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.
Go back to the previous code
modules/geometry-calculation.js
Outdated
| const proxyLine = new Line({point1:line.point1,point2: point}) | ||
| proxyLine.calculateSlope() | ||
| if (line.slope === proxyLine.slope) { | ||
| if (line.slope === proxyLine.slope) { |
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.
What happens if the slope of the line was not calculated yet
modules/geometry-calculation.js
Outdated
| if (line.slope === proxyLine.slope) { | ||
| proxyLine.calculateNOfLineFunction() | ||
| if (line.n === proxyLine.n2) { | ||
| if (line.n === proxyLine.n) { |
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.
What happens if the n of the line was not calculated yet?
| const { Line, getPointByY, calculateNOfLineFunction, getPointOnXAsis, getPointOnYAsis, getPointByX } = require('../../../modules/ecs6-class/line'); | ||
| const point = require('../../../modules/ecs6-class/point'); | ||
| const { Point } = require('../../../modules/ecs6-class/point'); | ||
| const mockConstructor = jest.fn(constructor); |
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.
this is totally wrong for mocks
gemtechd
left a comment
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.
cannot run the tests
something is not good in your package.json file (I don't have node_modules)
2 and where are the mocks?
| const Line = require('../../../modules/ecs6-class/line'); | ||
| const Point = require('../../../modules/ecs6-class/point'); | ||
| let line; | ||
| describe('constructor', () => { |
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.
where is the good test?
| line = (new Line({ point1: (new Point({ x: 1, y: 2 })), point2: (new Point({ x: 1, y: 10 })), n: 5, slope: 4 })) | ||
| expect(() => { line.calculateSlope() }).toThrow('cut with zero'); | ||
| }) | ||
| it('should be 3 when all is good', () => { |
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.
what sould be 3? maybe you'll change the values one day?
gemtechd
left a comment
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.
All other tests are finished?
| expect(line.point1).toEqual(new Point(1, 2)); | ||
| expect(line.point2).toEqual(new Point(4, 5)) | ||
| expect(line.n).toBe(4); | ||
| expect(line.slope).toBe(7); |
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.
more examples for good cases
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 would appreciate a more detailed explanation please.
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 teacher Can respond to this PR please?
est