-
Notifications
You must be signed in to change notification settings - Fork 0
Code review #1
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: master
Are you sure you want to change the base?
Code review #1
Changes from all commits
7b81267
33625c2
0b71e58
88a8c68
7900057
6eafa75
d9f9811
8c785c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,61 +1,79 @@ | ||
| const array = ['cat', 'dog', 'elephant', 'fox']; | ||
| const getNthElement = (index, array) => { | ||
| // your code here | ||
| return array[index % array.length]; | ||
| }; | ||
|
|
||
|
|
||
| const arrayToCSVString = array => { | ||
| // your code here | ||
| return array.toString(); | ||
| }; | ||
|
|
||
| const csvStringToArray = string => { | ||
| // your code here | ||
| return string.split(','); | ||
| }; | ||
|
|
||
| const addToArray = (element, array) => { | ||
| // your code here | ||
| console.log(array.push(element)); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is good practice to remove console.log() s from your code once you are finished. This is because you usually don't want to display data to users in the console from within your application/programme where possible. It could be sensitive information and it might also create a lot of noise in case there are other important messages or errors in the console.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just writing |
||
| }; | ||
|
|
||
| const addToArray2 = (element, array) => { | ||
| // your code here | ||
| return array.concat(element); | ||
| }; | ||
|
|
||
| const removeNthElement = (index, array) => { | ||
| // your code here | ||
| return array.splice(index, 1); | ||
| }; | ||
|
|
||
| const numbersToStrings = numbers => { | ||
| // your code here | ||
| return numbers.map(String); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really clever. +1 |
||
| }; | ||
|
|
||
| const uppercaseWordsInArray = strings => { | ||
| // your code here | ||
| return strings.map(string => string.toUpperCase()); | ||
| }; | ||
|
|
||
| const reverseWordsInArray = strings => { | ||
| // your code here | ||
| return strings.map(string => string.split('').reverse().join('')); | ||
| }; | ||
|
|
||
| const onlyEven = numbers => { | ||
| // your code here | ||
| return numbers.filter(x => x % 2 === 0); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works perfectly, the only suggestion I would have is usually we like to describe what a parameter is through it's naming, so instead of |
||
| }; | ||
|
|
||
| const removeNthElement2 = (index, array) => { | ||
| // your code here | ||
| }; | ||
| const newArray = [...array]; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of the spread operator. This is a really important and commonly used operator. |
||
| newArray.splice(index, 1); | ||
| return newArray; | ||
| } | ||
|
|
||
| const elementsStartingWithAVowel = strings => { | ||
| // your code here | ||
| }; | ||
| return strings.filter(string => (string.match(/^[aeiou]/gi))); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have one extra set of brackets her that I don't think you need. |
||
| } | ||
|
|
||
| const removeSpaces = string => { | ||
| // your code here | ||
| return string.replace(/\s+/g, ''); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice use of replace |
||
| }; | ||
|
|
||
| const sumNumbers = numbers => { | ||
| // your code here | ||
| return numbers.reduce((total, current) => total + current, 0) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice use of reduce |
||
| }; | ||
|
|
||
| const sortByLastLetter = strings => { | ||
| // your code here | ||
| return strings.sort((x, y) => x.charCodeAt(x.length - 1) - y.charCodeAt(y.length - 1)); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice use of character codes |
||
| }; | ||
|
|
||
| module.exports = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,62 +1,98 @@ | ||
| function negate(a) { | ||
| // your code here | ||
| }; | ||
| const negate = (a) => !a; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woo! Implicit returning :) |
||
|
|
||
| function both(a, b) { | ||
| // your code here | ||
| }; | ||
| const both = (a, b) => { | ||
| if (!a || !b) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use |
||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| function either(a, b) { | ||
| // your code here | ||
| }; | ||
| const either = (a, b) => { | ||
| if (a || b) { | ||
| return true; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same refactor here |
||
| } | ||
| return false; | ||
| } | ||
| const none = (a, b) => { | ||
| if (a || b) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this because it is a slightly different solution to what I expected, but it still works. I would use |
||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| function none(a, b) { | ||
| // your code here | ||
| }; | ||
| const one = (a, b) => { | ||
| if ((a && !b) || (b && !a)) { | ||
| return true; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use |
||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function one(a, b) { | ||
| // your code here | ||
| }; | ||
| const truthiness = (a) => { | ||
| if(a){ | ||
| return true; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use |
||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function truthiness(a) { | ||
| // your code here | ||
| }; | ||
| const isEqual = (a, b) => { | ||
| if (a === b){ | ||
| return true; | ||
| } | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just return |
||
| return false; | ||
| } | ||
|
|
||
| function isEqual(a, b) { | ||
| // your code here | ||
| }; | ||
|
|
||
| function isGreaterThan(a, b) { | ||
| // your code here | ||
| }; | ||
| const isGreaterThan = (a, b) => { | ||
| if (a > b){ | ||
| return true; | ||
| } | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, |
||
| return false; | ||
| } | ||
|
|
||
| function isLessThanOrEqualTo(a, b) { | ||
| // your code here | ||
| }; | ||
| const isLessThanOrEqualTo = (a, b) => { | ||
| if (a <= b){ | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isOdd(a) { | ||
| // your code here | ||
| }; | ||
| const isOdd = (a) => { | ||
| if (a % 2 === 1){ | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isEven(a) { | ||
| // your code here | ||
| }; | ||
| const isEven = (a) => { | ||
| if (a % 2 === 0){ | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isSquare(a) { | ||
| // your code here | ||
| }; | ||
| const isSquare = (a) => { | ||
| if (Math.sqrt(a) % 1 === 0){ | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function startsWith(char, string) { | ||
| // your code here | ||
| }; | ||
| const startsWith = (char, string) => { | ||
| if (string.startsWith(char) === true){ | ||
| return true; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here you can use |
||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function containsVowels(string) { | ||
| // your code here | ||
| }; | ||
|
|
||
| function isLowerCase(string) { | ||
| // your code here | ||
| }; | ||
| const containsVowels = (string) => /[aeiou]/gi.test(string); | ||
|
|
||
| const isLowerCase = (string) => { | ||
| if (string === string.toLowerCase()){ | ||
| return true; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your answers are all really good, you can just shorten them by getting rid of the if else statements like I have suggested above :) |
||
| } | ||
| return false; | ||
| } | ||
|
|
||
| module.exports = { | ||
| negate, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,57 +1,35 @@ | ||
| function add (a, b) { | ||
| // your code here | ||
| } | ||
| const add = (a, b) => a + b; | ||
|
|
||
| function subtract (a, b) { | ||
| // your code here | ||
| } | ||
| const subtract = (a, b) => a - b; | ||
|
|
||
| function multiply (a, b) { | ||
| // your code here | ||
| } | ||
| const multiply = (a, b) => a * b; | ||
|
|
||
| function divide (a, b) { | ||
| // your code here | ||
| } | ||
| const divide = (a, b) => a / b; | ||
|
|
||
| function power (a, b) { | ||
| // your code here | ||
| } | ||
| const power = (a, b) => a ** b; | ||
|
|
||
| function round (a) { | ||
| // your code here | ||
| } | ||
| const round = (a) => Math.round(a); | ||
|
|
||
| function roundUp (a) { | ||
| // your code here | ||
| } | ||
| const roundUp = (a) => Math.ceil(a); | ||
|
|
||
| function roundDown (a) { | ||
| // your code here | ||
| } | ||
| const roundDown = (a) => Math.floor(a); | ||
|
|
||
| function absolute (a) { | ||
| // your code here | ||
| } | ||
| const absolute = (a) => Math.abs(a); | ||
|
|
||
| function quotient (a, b) { | ||
| // your code here | ||
| } | ||
| const quotient = (a, b) => Math.trunc(a / b); | ||
|
|
||
| function remainder (a, b) { | ||
| // your code here | ||
| } | ||
| const remainder = (a, b) => a % b; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely perfect 👯 |
||
|
|
||
| module.exports = { | ||
| add, | ||
| subtract, | ||
| multiply, | ||
| divide, | ||
| power, | ||
| round, | ||
| roundUp, | ||
| roundDown, | ||
| absolute, | ||
| quotient, | ||
| remainder | ||
| } | ||
| add, | ||
| subtract, | ||
| multiply, | ||
| divide, | ||
| power, | ||
| round, | ||
| roundUp, | ||
| roundDown, | ||
| absolute, | ||
| quotient, | ||
| remainder | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,59 @@ | ||
| const createPerson = (name, age) => { | ||
| // your code here | ||
| this.name = name; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really good use of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most people have been writing object literals like |
||
| this.age = age; | ||
| return this; | ||
| }; | ||
|
|
||
| const getName = object => { | ||
| // your code here | ||
| return object.name; | ||
| }; | ||
|
|
||
| const getProperty = (property, object) => { | ||
| // your code here | ||
| return object[property]; | ||
| }; | ||
|
|
||
| const hasProperty = (property, object) => { | ||
| // your code here | ||
| return object[property]? true : false; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of a ternary here! +1 You could also us the hasOwnProperty method https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty |
||
| }; | ||
|
|
||
| const isOver65 = person => { | ||
| // your code here | ||
| return person.age > 65? true : false; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just shorten this to |
||
| }; | ||
|
|
||
| const getAges = people => { | ||
| // your code here | ||
| return people.map(ageArray => ageArray.age); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically we are mapping over the people array here, so each iteration is using a person. I would change the variable name from |
||
| }; | ||
|
|
||
| const findByName = (name, people) => { | ||
| // your code here | ||
| return people.find(person => person.name === name); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perfecto |
||
| }; | ||
|
|
||
| const findHondas = cars => { | ||
| // your code here | ||
| return cars.filter(carModel => carModel.manufacturer === 'Honda'); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is great. I would just suggest naming the variable |
||
| }; | ||
|
|
||
| const averageAge = people => { | ||
| // your code here | ||
| return people.reduce((a,b) => a + b.age, 0) / people.length; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice |
||
| }; | ||
|
|
||
| const createTalkingPerson = (name, age) => { | ||
| // your code here | ||
| return { | ||
| name: name, | ||
| age: age, | ||
| introduce: stranger => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! |
||
| return `Hi ${stranger}, my name is ${name} and I am ${age}!`; | ||
| } | ||
| }; | ||
| }; | ||
|
|
||
| module.exports = { | ||
|
|
||
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.
Nice