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

Array Home work #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaalyALkaraki
Copy link

Hello Edward,
First I would thank you for this exercise ,it was really nice to use J.S next to Html and CSS
this my first time and i hope it will not be the last ,I tried to solve it , I have some confusion in using the methods i fell that some of them are the same so i am trying to read more about them to understand it better
I'll be waiting for your review
Regards
Maaly ,

Copy link
Owner

@edwardbrowncross edwardbrowncross left a comment

Choose a reason for hiding this comment

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

I'm very impressed with this. Everything is correct and you have clearly thought carefully about what you have done. 💯 It's really good that you've been able to use filter and map correctly even though you weren't able to make it to the class on Saturday.

I've left a few comments in the code for your feedback.

👍

function onClearAll() {
todos.length = 0; // we can Set the array length to zero
// or we can replace an array with an empty array todos = [];
Copy link
Owner

Choose a reason for hiding this comment

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

It's good that you've considered multiple solutions!


if (newTodoText.length < 4) {
//process.exit(1);
Copy link
Owner

Choose a reason for hiding this comment

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

I think by now you must have discovered that this doesn't work in the browser. There are a number of differences between Node and web browser JS, as you'll find out later in the course.

if (newTodoText.length < 4) {
//process.exit(1);
alert(newTodoText + " It should be more than 3 characters long ");
Copy link
Owner

Choose a reason for hiding this comment

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

I like that you've used the alert function from later down in the exercise to solve a problem here too.

} else {
todos[index] = "X " + todos[index];
} //I think there will be a problem that we can't add new todo that starts with X because it
Copy link
Owner

Choose a reason for hiding this comment

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

Absolutely right! It's always good to consider the unintended consequences of what you've been asked to do. Do you have any ideas for how we could make this better?

alert(
todos.find(function(item) {
return !item.startsWith("X");
Copy link
Owner

Choose a reason for hiding this comment

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

This is the third time you have checked that a string starts with an X. What could you add to this code that would prevent you repeating yourself?

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