-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement _check #2107
base: GANONTHA
Are you sure you want to change the base?
Implement _check #2107
Conversation
Thank you for the submission, @GANONTHA! I'll review your code shortly, hang tight. |
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 a great attempt, @GANONTHA!
I would like to request a few changes before merging your work. Please review my comments below and make the appropriate changes to your code.
After you update your code locally, follow the instructions to save your changes locally and push your changes to your fork.
When you push your changes to your fork, I'll come back for another review.
There are 36 style guide violations in your contribution. I've marked them with inline comments for your convenience.
Please revisit your code and follow the style guide best practices.
Hint: You might be able to fix some issues automatically by running npm run lint -- --fix
All the tests are passing. Nice job!
// DRY up the codebase with this function | ||
// First, move the duplicate error checking code here | ||
if (typeof x !== "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.
Strings must use singlequote.
(learn more)
if (typeof x !== "number") { | ||
throw new TypeError(`${x} is not a number`); | ||
} | ||
if (typeof y !== "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.
Strings must use singlequote.
(learn more)
@@ -1,20 +1,20 @@ | |||
/* eslint-disable no-unused-expressions */ | |||
const calculator = require('./calculator'); | |||
const calculator = require("./calculator"); |
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.
Strings must use singlequote.
(learn more)
|
||
describe.skip('_check', () => { | ||
describe("_check", () => { |
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.
Strings must use singlequote.
(learn more)
beforeEach(() => { | ||
sinon.spy(calculator, '_check'); | ||
sinon.spy(calculator, "_check"); |
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.
Strings must use singlequote.
(learn more)
describe('subtract', () => { | ||
it('should throw a TypeError if arguments are not numbers', () => { | ||
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { |
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.
Strings must use singlequote.
(learn more)
it('should throw a TypeError if arguments are not numbers', () => { | ||
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { | ||
it("should throw a TypeError if arguments are not numbers", () => { |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { | ||
it("should throw a TypeError if arguments are not numbers", () => { | ||
expect(() => calculator.subtract(40, "2")).to.throw(TypeError); |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract(40, [])).to.throw(TypeError); | ||
expect(() => calculator.subtract(40, {})).to.throw(TypeError); | ||
expect(() => calculator.subtract('40', 2)).to.throw(TypeError); | ||
expect(() => calculator.subtract("40", 2)).to.throw(TypeError); |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract([], 2)).to.throw(TypeError); | ||
expect(() => calculator.subtract({}, 2)).to.throw(TypeError); | ||
}); | ||
|
||
it('should subtract two positive numbers', () => { | ||
it("should subtract two positive numbers", () => { |
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.
Strings must use singlequote.
(learn more)
Thanks for the changes, @GANONTHA. I'm reviewing them now. |
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 a great attempt, @GANONTHA!
I would like to request a few changes before merging your work. Please review my comments below and make the appropriate changes to your code.
After you update your code locally, follow the instructions to save your changes locally and push your changes to your fork.
When you push your changes to your fork, I'll come back for another review.
There are 36 style guide violations in your contribution. I've marked them with inline comments for your convenience.
Please revisit your code and follow the style guide best practices.
Hint: You might be able to fix some issues automatically by running npm run lint -- --fix
All the tests are passing. Nice job!
// DRY up the codebase with this function | ||
// First, move the duplicate error checking code here | ||
if (typeof x !== "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.
Strings must use singlequote.
(learn more)
if (typeof x !== "number") { | ||
throw new TypeError(`${x} is not a number`); | ||
} | ||
if (typeof y !== "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.
Strings must use singlequote.
(learn more)
@@ -1,20 +1,20 @@ | |||
/* eslint-disable no-unused-expressions */ | |||
const calculator = require('./calculator'); | |||
const calculator = require("./calculator"); |
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.
Strings must use singlequote.
(learn more)
|
||
describe.skip('_check', () => { | ||
describe("_check", () => { |
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.
Strings must use singlequote.
(learn more)
beforeEach(() => { | ||
sinon.spy(calculator, '_check'); | ||
sinon.spy(calculator, "_check"); |
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.
Strings must use singlequote.
(learn more)
describe('subtract', () => { | ||
it('should throw a TypeError if arguments are not numbers', () => { | ||
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { |
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.
Strings must use singlequote.
(learn more)
it('should throw a TypeError if arguments are not numbers', () => { | ||
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { | ||
it("should throw a TypeError if arguments are not numbers", () => { |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract(40, '2')).to.throw(TypeError); | ||
describe("subtract", () => { | ||
it("should throw a TypeError if arguments are not numbers", () => { | ||
expect(() => calculator.subtract(40, "2")).to.throw(TypeError); |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract(40, [])).to.throw(TypeError); | ||
expect(() => calculator.subtract(40, {})).to.throw(TypeError); | ||
expect(() => calculator.subtract('40', 2)).to.throw(TypeError); | ||
expect(() => calculator.subtract("40", 2)).to.throw(TypeError); |
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.
Strings must use singlequote.
(learn more)
expect(() => calculator.subtract([], 2)).to.throw(TypeError); | ||
expect(() => calculator.subtract({}, 2)).to.throw(TypeError); | ||
}); | ||
|
||
it('should subtract two positive numbers', () => { | ||
it("should subtract two positive numbers", () => { |
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.
Strings must use singlequote.
(learn more)
This is the summary of my contribution to the open source project
Basically I did two things. The first was to implement the fuhct _check to avoid repetitions in the code by removing all the redundant codes from different scopes and saving inside a single function. This allowed me to make the code DRY.
Next I touch a bit the test file so that the test runs. There were some typos in the test.
And also, I have installed some dependencies in order to run the code on me local machine.
I the end three (3) files have been changes:
1 -> calculator.js
2 -> calculator.test.js
3 -> package-lock.json