Code review by anywhereiromy · Pull Request #1 · codeandstuff-manchester/javascript-basics

anywhereiromy · 2021-11-30T18:15:07Z

No description provided.

anywhereiromy · 2021-11-30T18:15:23Z

src/arrays.js

+const array = ['cat', 'dog', 'elephant', 'fox'];
 const getNthElement = (index, array) => {
-  // your code here
+  return array[index % array.length];


anywhereiromy · 2021-11-30T18:17:03Z

src/arrays.js


 const addToArray = (element, array) => {
  // your code here
+  console.log(array.push(element));


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.

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

anywhereiromy · 2021-11-30T18:18:39Z

src/arrays.js


 const numbersToStrings = numbers => {
  // your code here
+  return numbers.map(String);


This is really clever. +1

anywhereiromy · 2021-11-30T18:20:43Z

src/arrays.js


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


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.

anywhereiromy · 2021-11-30T18:21:03Z

src/arrays.js

 const removeNthElement2 = (index, array) => {
  // your code here
-};
+  const newArray = [...array];


Nice use of the spread operator. This is a really important and commonly used operator.

anywhereiromy · 2021-11-30T18:22:18Z

src/arrays.js

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


You have one extra set of brackets her that I don't think you need. return strings.filter(string => string.match(/^[aeiou]/gi));

anywhereiromy · 2021-11-30T18:22:33Z

src/arrays.js


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


nice use of replace

anywhereiromy · 2021-11-30T18:22:47Z

src/arrays.js


 const sumNumbers = numbers => {
  // your code here
+  return numbers.reduce((total, current) => total + current, 0)


nice use of reduce

anywhereiromy · 2021-11-30T18:23:02Z

src/arrays.js


 const sortByLastLetter = strings => {
  // your code here
+  return strings.sort((x, y) => x.charCodeAt(x.length - 1) - y.charCodeAt(y.length - 1));


nice use of character codes

anywhereiromy · 2021-11-30T18:23:21Z

src/booleans.js

-function negate(a) {
-  // your code here
-};
+const negate = (a) => !a;


Woo! Implicit returning :)

anywhereiromy · 2021-11-30T18:24:32Z

src/booleans.js

-  // your code here
-};
+const both = (a, b) => {
+  if (!a || !b) {


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

anywhereiromy · 2021-11-30T18:24:45Z

src/booleans.js

-};
+const either = (a, b) => {
+  if (a || b) {
+    return true;


Same refactor here

anywhereiromy · 2021-11-30T18:26:56Z

src/booleans.js

+  return false;
+}
+const none = (a, b) => {
+  if (a || b) {


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.

anywhereiromy · 2021-11-30T18:27:57Z

src/booleans.js

-};
+const truthiness = (a) => {
+  if(a){
+    return true;


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

anywhereiromy · 2021-11-30T18:28:49Z

src/booleans.js

-};
+const one = (a, b) => {
+  if ((a && !b) || (b && !a)) {
+    return true;


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

anywhereiromy · 2021-11-30T18:29:22Z

src/booleans.js

+const isEqual = (a, b) => {
+  if (a === b){
+    return true;
+  }


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

anywhereiromy · 2021-11-30T18:29:40Z

src/booleans.js

+const isGreaterThan = (a, b) => {
+  if (a > b){
+    return true;
+  }


Same here, return a > b will also work.

anywhereiromy · 2021-11-30T18:31:02Z

src/booleans.js

-};
+const startsWith = (char, string) => {
+  if (string.startsWith(char) === true){
+    return true;


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

anywhereiromy · 2021-11-30T18:31:42Z

src/booleans.js

+
+const isLowerCase = (string) => {
+  if (string === string.toLowerCase()){
+    return true;


Your answers are all really good, you can just shorten them by getting rid of the if else statements like I have suggested above :)

anywhereiromy · 2021-11-30T18:32:04Z

src/numbers.js

-function remainder (a, b) {
-  // your code here
-}
+const remainder = (a, b) => a % b;


Absolutely perfect 👯

anywhereiromy · 2021-11-30T18:32:58Z

src/objects.js


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


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

anywhereiromy · 2021-11-30T18:33:19Z

src/objects.js


 const isOver65 = person => {
  // your code here
+  return person.age > 65? true : false;


You can just shorten this to person.age > 65

anywhereiromy · 2021-11-30T18:34:22Z

src/objects.js


 const getAges = people => {
  // your code here
+  return people.map(ageArray => ageArray.age);


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.

anywhereiromy · 2021-11-30T18:34:32Z

src/objects.js


 const findByName = (name, people) => {
  // your code here
+  return people.find(person => person.name === name);


anywhereiromy · 2021-11-30T18:35:20Z

src/objects.js


 const findHondas = cars => {
  // your code here
+  return cars.filter(carModel => carModel.manufacturer === 'Honda');


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.

anywhereiromy · 2021-11-30T18:35:27Z

src/objects.js


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


anywhereiromy · 2021-11-30T18:35:42Z

src/objects.js

+  return {
+    name: name,
+    age: age,
+    introduce: stranger => {


anywhereiromy · 2021-11-30T18:36:02Z

src/objects.js

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


Really good use of this here.

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

anywhereiromy · 2021-11-30T18:37:21Z

src/strings.js

-function firstCharacter (string) {
-  // your code here
-};
+const firstCharacter = (firstAlp) => firstAlp[0];


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

anywhereiromy · 2021-11-30T18:37:35Z

src/strings.js

-function firstCharacters (string, n) {
-  // your code here
-};
+const firstCharacters = (string, n) => string.slice(0, n);


Perfect functions :D

anywhereiromy

Really good work! Just a few minor tweaks suggested.

JMoss89 added 8 commits November 24, 2021 08:04

completed strings.js test

7b81267

completed numbers.js test

33625c2

completed booleans.js test

0b71e58

completed arrays.js test

88a8c68

completed objects.js test

7900057

refactored numbers.js

6eafa75

refactored strings.js

d9f9811

refactored booleans.js

8c785c5

anywhereiromy commented Nov 30, 2021

View reviewed changes

src/booleans.js

function negate(a) {

// your code here

};

const negate = (a) => !a;

Copy link

Collaborator Author

anywhereiromy Nov 30, 2021

Choose a reason for hiding this comment

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

Woo! Implicit returning :)

anywhereiromy commented Nov 30, 2021

View reviewed changes

src/booleans.js

};

const either = (a, b) => {

if (a || b) {

return true;

Copy link

Collaborator Author

anywhereiromy Nov 30, 2021

Choose a reason for hiding this comment

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

Same refactor here

anywhereiromy commented Nov 30, 2021

View reviewed changes

src/objects.js

return {

name: name,

age: age,

introduce: stranger => {

Copy link

Collaborator Author

anywhereiromy Nov 30, 2021

Choose a reason for hiding this comment

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

Awesome!

anywhereiromy commented Nov 30, 2021

View reviewed changes

Conversation

anywhereiromy commented Nov 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anywhereiromy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants