Conversation
There was a problem hiding this comment.
Hey Kate 👋
Well done in completing this track 🎈You have good solutions to the katas and your code tends to be nicely succinct.
A couple of comments:
- we try to make our code readable and maintainable, and spacing, organization, and variable naming matter.
- fizzbuzz is 1/3 implemented, if you'd like to pair program the remaining of it I'd be happy to do so with you. This kata is one that many companies use for tech tests, believe it or not!
| @@ -1,5 +1,5 @@ | |||
| const reachDestination = (distance, speed) => { | |||
|
|
|||
| const reachDestination = (d, s) => { | |||
There was a problem hiding this comment.
Good and succinct implementation. Ideally, we call our variables more than single vowels, like distance instead of d and speed intead of s.
| @@ -1,5 +1,6 @@ | |||
| const joinNames = (namesObj) => { | |||
There was a problem hiding this comment.
👌 A comment on the variable naming, x makes sense in a situation where we're talking about spacial coordinates, but on an array of names not as much. Maybe we can use name instead?
| @@ -1,5 +1,5 @@ | |||
| const numberToReversedDigits = (number) => { | |||
There was a problem hiding this comment.
Excellent, good use of string methods, nicely concatenated too 👌
| @@ -1,5 +1,28 @@ | |||
| const humanCatDogYears = (number) => { | |||
There was a problem hiding this comment.
Good job on this one. Ideally, we should try to make sure our code is well structured (indentation, spacing and variables well named).
| @@ -1,5 +1,28 @@ | |||
| const humanCatDogYears = (number) => { | |||
| let tmp = number; | |||
There was a problem hiding this comment.
No need to assign number to temp as we can use number throughout the algorithm.
| @@ -1,5 +1,9 @@ | |||
| const getEmployerRole = (employeeName, employees) => { | |||
There was a problem hiding this comment.
You're very close in nailing the implementation of this one:
- first error is
employee is not definedand that's because it needs to be declared likefor (let employee of employees)💡 notice thelet - then our comparison in the
ifstatement:employeeis the whole object that includesnameandrole, so we might want to write it likeif (employee.name === employeeName) - finally, we want to return the employee role, and we can do that by
return employee.role
| @@ -1,5 +1,9 @@ | |||
| const booleanToWord = (boolean) => { | |||
There was a problem hiding this comment.
If you'd like something some succinct try a ternary operator like return Boolean(boolean) ? 'Yes' : 'No'
| }, { | ||
| name: 'Maggie' | ||
| }]; | ||
| expect(joinNames(data)).toEqual('Bart, Lisa & Maggie'); |
| const getEmployerRole = require('../src/kata7.getEmployerRole'); | ||
|
|
||
| describe('getEmployerRole', () => { | ||
| const employees = [{ |
There was a problem hiding this comment.
For this test, it would have been perfect to have at least another element in the array, to make sure that our function is actually choosing one employee over another.
| @@ -1,19 +1,21 @@ | |||
| const { fizzBuzz } = require('../src'); | |||
| import { fizzBuzz } from '../src/kata1.fizzBuzz'; | |||
There was a problem hiding this comment.
As we're working in node, the import {} from "" syntax doesn't work. We need to use the require syntax here as we do in all the other tests. This is why our tests for fizzbuzz don't run at all atm with the error TypeError: (0 , _kata.fizzBuzz) is not a function
2 participants
No description provided.