Skip to content
This repository was archived by the owner on Jan 14, 2024. It is now read-only.

Glasgow Class 6 - Rahma Berhan - JavaScript_1 - Week_4 #231

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RahmaB1
Copy link

@RahmaB1 RahmaB1 commented Mar 28, 2023

No description provided.

Copy link

@annacollins85 annacollins85 left a comment

Choose a reason for hiding this comment

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

Great work! Your code is well formatted and you have used good variable names throughout which makes it very easy to read and follow. This is really important for other developers to be able to work with your code. Well done! Just added a few comments for improvement but there weren't many! 👏

Comment on lines +9 to +12
return names.find(function (name) {
if (name.startsWith("A") && name.length > 7) return name;
});
}

Choose a reason for hiding this comment

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

Great! 👏

@@ -16,6 +16,10 @@ let students = ["Islam", "Lesley", "Harun", "Rukmini"];
let mentors = ["Daniel", "Irina", "Mozafar", "Luke"];

let pairs = pairsByIndex.map(function (indexes) {
if (pairsByIndex.some((element) => element === null)) {

Choose a reason for hiding this comment

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

👏

Choose a reason for hiding this comment

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

Great use of arrow functions

@@ -5,7 +5,7 @@
let students = ["Omar", "Austine", "Dany", "Swathi", "Lesley", "Rukmini"];
let group = ["Austine", "Dany", "Swathi", "Daniel"];

let groupIsOnlyStudents; // complete this statement
let groupIsOnlyStudents = group.every((i) => students.includes(i));

Choose a reason for hiding this comment

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

This is good but I would use a more descriptive name, e.g.

Suggested change
let groupIsOnlyStudents = group.every((i) => students.includes(i));
let groupIsOnlyStudents = group.every((name) => students.includes(name));

Comment on lines +11 to +13
let pairsByIndex = pairsByIndexRaw.filter(
(element) => Array.isArray(element) && element.length === 2
);

Choose a reason for hiding this comment

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

👏

let firstFive; // complete this statement
let lastFive; // complete this statement
let firstFive = everyone.slice(0, 5); // complete this statement
let lastFive = everyone.slice(everyone.length - 5, everyone.length); // complete this statement

Choose a reason for hiding this comment

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

There's an easier way of doing this

Suggested change
let lastFive = everyone.slice(everyone.length - 5, everyone.length); // complete this statement
let lastFive = everyone.slice(-5)

@@ -44,7 +51,16 @@ Write a function that:
- Numbers greater 100 must be replaced with 100.
*/

function formatPercentage() {
function formatPercentage(numbers) {
numbersWithPercentages = numbers.map(addPercentage);

Choose a reason for hiding this comment

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

Great!

//ERROR --->>>> indSafeOxygenLevel function returns undefined if no valid planets found

function findSafeOxygenLevel(array) {
if (array.some((number) => number.includes("%"))) {

Choose a reason for hiding this comment

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

Why do we need this check here? Will it not always be the case?

Comment on lines +26 to +33
function checkLevel(number) {
if (number > 19.5 && number < 23.5) {
return true;
} else {
return false;
}
}
}

Choose a reason for hiding this comment

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

You could make this simpler by:

Suggested change
function checkLevel(number) {
if (number > 19.5 && number < 23.5) {
return true;
} else {
return false;
}
}
}
function checkLevel(number) {
return (number > 19.5 && number < 23.5);
}

return families.filter(familyNameChecker);
}

function familyNameChecker(familyName) {

Choose a reason for hiding this comment

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

You could have used array.split(' ') to create an array and then check the last item in the array is "family" and check the first one begins with "A" but is not just "A"

Comment on lines +10 to +19
function getEligibleStudents(attendanceData) {
let studentsNameWith8Classes = [];
attendanceData.forEach((student) => {
if (student[1] >= 8) {
studentsNameWith8Classes.push(student[0]);
return true;
}
});
return studentsNameWith8Classes;
}

Choose a reason for hiding this comment

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

Nice!

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

Successfully merging this pull request may close these issues.

None yet

2 participants