Skip to content
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

Code review #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Code review #1

wants to merge 8 commits into from

Conversation

anywhereiromy
Copy link
Collaborator

No description provided.

const getNthElement = (index, array) => {
// your code here
return array[index % array.length];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

};

const addToArray = (element, array) => {
// your code here
console.log(array.push(element));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just writing array.push(element) here would be great

};

const numbersToStrings = numbers => {
// your code here
return numbers.map(String);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really clever. +1

};

const onlyEven = numbers => {
// your code here
return numbers.filter(x => x % 2 === 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 x here, I would probably write number or num. Just because it is slightly more descriptive, as the letter x could represent anything.

};

const removeNthElement2 = (index, array) => {
// your code here
};
const newArray = [...array];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.


const elementsStartingWithAVowel = strings => {
// your code here
};
return strings.filter(string => (string.match(/^[aeiou]/gi)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. return strings.filter(string => string.match(/^[aeiou]/gi));


const removeSpaces = string => {
// your code here
return string.replace(/\s+/g, '');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice use of character codes

function negate(a) {
// your code here
};
const negate = (a) => !a;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo! Implicit returning :)

// your code here
};
const both = (a, b) => {
if (!a || !b) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use return Boolean(a || b) or return !!(a || b) here if you wanted to get rid of the if else statement.

};
const either = (a, b) => {
if (a || b) {
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same refactor here

return false;
}
const none = (a, b) => {
if (a || b) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 !a && !b here.

};
const truthiness = (a) => {
if(a){
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Boolean(a) or !!(a) here to get rid of the if else statement

};
const one = (a, b) => {
if ((a && !b) || (b && !a)) {
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use return Boolean(((a && !b) || (b && !a))) or return !!((a && !b) || (b && !a)) here to get rid of the if/else statement.

const isEqual = (a, b) => {
if (a === b){
return true;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just return a === b here as the comparison logic evaluates to a boolean already. :)

const isGreaterThan = (a, b) => {
if (a > b){
return true;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, return a > b will also work.

};
const startsWith = (char, string) => {
if (string.startsWith(char) === true){
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here you can use return string.startsWith(char) as that method already returns true or false


const isLowerCase = (string) => {
if (string === string.toLowerCase()){
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 :)

function remainder (a, b) {
// your code here
}
const remainder = (a, b) => a % b;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely perfect 👯

};

const hasProperty = (property, object) => {
// your code here
return object[property]? true : false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just shorten this to person.age > 65

};

const getAges = people => {
// your code here
return people.map(ageArray => ageArray.age);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 ageArray to person to be more accurate.

};

const findByName = (name, people) => {
// your code here
return people.find(person => person.name === name);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I would just suggest naming the variable car instead of carModel to be more precise as the variable actually contains more than just information about the model.

};

const averageAge = people => {
// your code here
return people.reduce((a,b) => a + b.age, 0) / people.length;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

return {
name: name,
age: age,
introduce: stranger => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@@ -1,41 +1,59 @@
const createPerson = (name, age) => {
// your code here
this.name = name;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good use of this here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most people have been writing object literals like { name, age }, but both work.

function firstCharacter (string) {
// your code here
};
const firstCharacter = (firstAlp) => firstAlp[0];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably rename firstAlp to be string here as the tests can input any string.

function firstCharacters (string, n) {
// your code here
};
const firstCharacters = (string, n) => string.slice(0, n);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect functions :D

Copy link
Collaborator Author

@anywhereiromy anywhereiromy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work! Just a few minor tweaks suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants