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

LONDON_10 || SAIM KORKMAZ || JS1-4 #230

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

Conversation

nsaimk
Copy link

@nsaimk nsaimk commented Mar 25, 2023

Mentor: @berkeli
Mates: @mabalal , @Mr-DEM1R

@@ -3,15 +3,17 @@ Write a function that:
- Accepts an array as a parameter.
- Returns a new array containing the first five elements of the passed array.
*/
function first5() {
function first5(arr) {
return arr.filter((element, index) => index < 5);
Copy link

Choose a reason for hiding this comment

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

This is not a good approach, filtering is an expensive operation that iterates over each element in the array. If array has n elements, it will result in n operations.

You can get the first 5 elements with 1 operation - arr.slice(0, 5);

@@ -33,7 +36,9 @@ Write a function that:
- Returns a new array containing the same elements, but without the element at the passed index.
*/

function remove() {
function remove(arr, index) {
const newArr = arr.filter((v, i) => index == i);
Copy link

@berkeli berkeli Mar 27, 2023

Choose a reason for hiding this comment

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

Similar comment as before, we should not use filter if we know which element we don't need:

const removedElement = arr.splice(index,1)
return arr // this will not contain removedElement

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

This modifies the source array, so if you want to avoid that - you can create a copy via const arr1 = [...arr];

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

function formatPercentage() {
function formatPercentage(arr) {
const formattedPercentages = numbers.map((number) => {
Copy link

Choose a reason for hiding this comment

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

Suggested change
const formattedPercentages = numbers.map((number) => {
const formattedPercentages = arr.map((number) => {

numbers is not defined here and also your tests are not passing for this function.

I'd suggest using Math.round(num * 100) / 100 instead of toFixed()

if (number > 100) {
number = 100;
}
return (Math.round(number * 100) / 100).toFixed(2) + "%";
Copy link

Choose a reason for hiding this comment

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

Suggested change
return (Math.round(number * 100) / 100).toFixed(2) + "%";
return (Math.round(number * 100) / 100)+ "%";

toFixed is not needed here, it will already round to 2 decimal places.

Comment on lines +15 to 17
function sortArray(arr) {
return arr.sort();
}
Copy link

Choose a reason for hiding this comment

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

Some tests for this function are failing because it modifies the passed in array. I'd suggest making a copy of the array and sorting that, e.g [...arr].sort();

Comment on lines +26 to +30
if (berryArray.every(v => v == "pink")) {
return "Bush is safe to eat from";
} else {
return "Toxic! Leave bush alone!";
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (berryArray.every(v => v == "pink")) {
return "Bush is safe to eat from";
} else {
return "Toxic! Leave bush alone!";
}
if (berryArray.every(v => v == "pink")) {
return "Bush is safe to eat from";
}
return "Toxic! Leave bush alone!";

We can skip the else statement for a cleaner code

Comment on lines +19 to +21
return (result = arr.filter(
(v) => v.startsWith("A") && v.includes("family")
));
Copy link

Choose a reason for hiding this comment

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

Can you explain why you need result here?

Comment on lines +10 to +20
function getEligibleStudents(arr) {
let result = [];

for (let i = 0; i < arr.length; i++) {
if (arr[i][1] > 7) {
result.push(arr[i][0])
}
}

return result
}
Copy link

Choose a reason for hiding this comment

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

This works fine, but since this week was focused on array methods - it would be nice if you can rewrite with .filter() and .map()

Comment on lines +132 to +138
let result = [];
for (let i = 0; i < locations.length; i++) {
if (locations[i].includes(transportMode)){
result.push(locations[i][0])
}
}
return result
Copy link

Choose a reason for hiding this comment

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

I think the idea here was to use the functions you wrote earlier (getLocationName and isAccessibleByTransportMode)

also, you can use.filter()and .map()

Copy link

@berkeli berkeli left a comment

Choose a reason for hiding this comment

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

Nice work, but I see there are multiple exercises missing in the 1-exercises folder.

@berkeli berkeli added the reviewed A mentor has reviewed this code label Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
reviewed A mentor has reviewed this code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants