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

Osman Elsahib- TDD #68

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

Osman Elsahib- TDD #68

wants to merge 4 commits into from

Conversation

elsahib
Copy link

@elsahib elsahib commented Sep 2, 2020

I missed one of the tests in my last PR, so here I'm submitting them again to get feedback on them.
Thanks.

Copy link

@Ashaghel Ashaghel left a comment

Choose a reason for hiding this comment

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

Finished Passing test!

module.exports = function(numbers) {};
module.exports = function(numbers) {
return numbers.map((number)=> number +1)
};
Copy link

Choose a reason for hiding this comment

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

Nice one

var getWordLengths = function(someWords) {};
var getWordLengths = function (someWords) {
return someWords.map(word => word.length)
};
Copy link

Choose a reason for hiding this comment

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

Same here 👍

return numbers.reduce(function (a, b) {
return a + b;
}, 0);
}
Copy link

Choose a reason for hiding this comment

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

Perfect

@@ -1,3 +1,10 @@
function findNeedle(words) {}
function findNeedle(words) {
let result;
Copy link

Choose a reason for hiding this comment

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

if you look at the test, you have two arguments passed, expecting two parameters.

function factorial(int) {}
function factorial(int) {
// let result = 1, i = int;
if (int === 0 || int === 1)
Copy link

Choose a reason for hiding this comment

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

while int is not reserved anymore in JS 5, it is reserved word in previous versions afaik
it's also reserved in many other languages. so while parameter is coming as int, it can be useful to save it in another variable

Copy link

Choose a reason for hiding this comment

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

Also this will save you the trouble of the first conditions, as you can initialize it with 1

let maxIndex = array.indexOf(actualMax)
array.splice(maxIndex, 1)
return Math.max(...array)
}
Copy link

Choose a reason for hiding this comment

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

nice work, and works fine
the only a couple of issues, the complexity is high here
You needed to loop through the array twice to get the max
also what if max number is duplicated ?

numArray.push(element)
}
});
let sum = numArray.reduce(function (a, b) {
Copy link

Choose a reason for hiding this comment

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

Also nicely done
you can make it fast using one loop also to do both checking if it's number and adding it

result["Land Rover"] = rover
result.Toyota = toyota

return result
Copy link

Choose a reason for hiding this comment

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

Really good work
you can add more test to make it fail
and move from hard-coding the car modules to program them in

@@ -0,0 +1,7 @@
function paintCars(array, color) {
// I didn't understand what is required to be done so I hardcoded the index
Copy link

Choose a reason for hiding this comment

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

this is a tricky one
they expect you to change the color of the Ford model (or first car) to the color they choose

Copy link

Choose a reason for hiding this comment

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

remember it should not change the original object colour
if you stuck let me know for more hints

@@ -0,0 +1,4 @@
function cities(array, func) {
return array.map(item => func(item))
Copy link

Choose a reason for hiding this comment

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

correct
also when writing map, you don't need to create it as array function if you're passign a function
something like
array.map(func)

});

return greeting;
return myarray;
Copy link

Choose a reason for hiding this comment

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

nice work, while forEach is pushing to another array (not quite its role)
something like map will help you better

// Assert
expect(result).toEqual(expected)
Copy link

Choose a reason for hiding this comment

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

nice work

} else {
result.push("_");
result.push(character);
}
Copy link

Choose a reason for hiding this comment

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

The idea for those tests, is to add more tests to make your functions work better. So try to do that and we can review them later on

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