-
Notifications
You must be signed in to change notification settings - Fork 90
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
kata 1 to four #2
base: main
Are you sure you want to change the base?
Conversation
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 the tests are passing which is the overall goal :) well done, nice work :)
I have added some comments below it is mainly on the formatting. The indentation is off on a lots of pages. There are extra lines left in the code as well and inconsistency with the '' and "" quotation marks. I would suggest to stick with the "" and that is because the single quotation marks are also used in strings for example: "Tom's mom" would work but 'Tom's mom' would not work because the middle ' would close the string.
@@ -1,11 +1,18 @@ | |||
const { fizzBuzz } = require("../src"); | |||
|
|||
describe("fizzBuzz", () => { | |||
test("returns Fizz when passed a multiple of 3", () => {}); | |||
it("returns Fizz when passed a multiple of 3", () => {}); | |||
expect(fizzBuzz(3)).toBe('Fizz') |
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 test cases' expect blocks are not in the correct position in this file.
They are after the it block instead of being in the curly braces.
The tests will still pass if they are correct but they wont belong to the correct block, with the description.
Should be like this:
it("returns Fizz when passed a multiple of 3", () => {
expect(fizzBuzz(3)).toBe('Fizz')
});
|
||
test("returns Buzz when passed a multiple of 5", () => {}); | ||
it("returns Buzz when passed a multiple of 5", () => {}); | ||
expect(fizzBuzz(5)).toBe('Buzz') |
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.
Same as above, expect is in the wrong place
|
||
test("returns FizzBuzz when passed a multiple 3 and 5", () => {}); | ||
it("returns FizzBuzz when passed a multiple 3 and 5", () => {}); | ||
expect(fizzBuzz(15)).toBe('FizzBuzz') |
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.
Same as above all these expects are in the wrong place
|
||
test("returns the number when it isn't a multiple of 3 or 5", () => {}); | ||
it("returns the number when it isn't a multiple of 3 or 5", () => {}); | ||
expect(fizzBuzz(4)).toBe(4) |
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.
Expects are in the wrong place
test("returns No when passed false", () => { | ||
expect(booleanToWord(false)).toBe("No"); | ||
}); | ||
|
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 extra line can be deleted for better formatting.
@@ -1,5 +1,17 @@ | |||
const { joinNames } = require("../src"); | |||
|
|||
describe("joinNames", () => { | |||
test("returns string of names, seperated by commas and an ampersand", () => {}); | |||
test("returns string of names, seperated by commas and an ampersand", () => { | |||
const names1 = [{name: 'Homer'}, {name: 'Marge'}, {name: 'Bart'}, {name: 'Lisa'}, {name: 'Maggie'}]; |
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.
When there is an array of objects and it is a bit long, you could reformat it like this:
const names1 = [
{ name: "Homer" },
{ name: "Marge" },
{ name: "Bart" },
{ name: "Lisa" },
{ name: "Maggie" },
];
To make it look a little bit tidier, and easier to read.
Also at some place you use '' and at some place you are using "". For better consistency I would recommend to use "" everywhere.
@@ -1,3 +1,6 @@ | |||
const numberToReversedDigits = (number) => {}; | |||
const numberToReversedDigits = (number) => { | |||
const numArray = number.toString().split("").reverse().map(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.
missing indentation.
@@ -1,3 +1,8 @@ | |||
const reachDestination = (distance, speed) => {}; | |||
const reachDestination = (distance, speed) => { | |||
const reachTime = distance / speed; |
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.
Line 2,3,4,5 are missing indentation
@@ -1,5 +1,21 @@ | |||
const { getEmployerRole } = require("../src"); | |||
|
|||
const employees = [ | |||
{ | |||
name: 'Satti', |
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 recommend to use "" - double quotations. You used doubles throughout the application, also it is recommended to use "" and that is because as you can see on line 19, single quote is also used in strings. (employee's)
} | ||
} | ||
return "Employee not found"; | ||
|
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 extra line could be deleted (line 1 and line 9).
The for loop's indentation is a bit too much, could be less like this:
const getEmployerRole = (employeeName, employees) => {
for (let i = 0; i < employees.length; i++) {
if (employees[i].name === employeeName) {
return employees[i].role;
}
}
return "Employee not found";
};
module.exports = getEmployerRole;
No description provided.