Open zacharyweston opened 8 years ago
Looks good. Everything works, which is the most important thing. :smiley: I always like to find some room for improvement though, so here it is:
Some of the comparisons we're making are a bit brain-bending. Take this for example:
!clickedElement.checked === false
Translated into English, that reads as Is it true that it is false that clickedElement is not checked? It takes my mind a moment to figure out what's going on there. :stuck_out_tongue: When we break it down, what that is really asking is, Is clickedElement checked?. So that can be rewritten as simply:
clickedElement.checked
Similarly, whenever you have === true
, like this:
!clickedElement.checked === true
Then you can always leave off the === true
and simply write:
!clickedElement.checked
But! It gets even better than this. Let's take a look at that code in its greater context:
if (clickedElement.className === 'checkbox' && !clickedElement.checked === false) {
todos[indexNumber].isComplete = true;
}
if (clickedElement.className === 'checkbox' && !clickedElement.checked === true) {
todos[indexNumber].isComplete = false;
}
We're checking if the className
of the clickedElement
is 'checkbox'
twice, which isn't really necessary, so let's instead start with:
if (clickedElement.className === 'checkbox') {
// ...
}
Then adding our simplified if statement:
if (clickedElement.className === 'checkbox') {
if (clickedElement.checked) {
todos[indexNumber].isComplete = true
}
if (!clickedElement.checked) {
todos[indexNumber].isComplete = false
}
}
And now we can see that we're checking if something is true, then if it is, doing one thing. Then we check if it's not true, and if it is, then we do another thing. That's a classic case for an if-else:
if (clickedElement.className === 'checkbox') {
if (clickedElement.checked) {
todos[indexNumber].isComplete = true
} else {
todos[indexNumber].isComplete = false
}
}
But you know what? It gets even better than this. Let's take a closer look at what we're doing. We're setting todos[indexNumber].isComplete
to either true or false, depending on whether clickedElement.checked
is true or false, respectively. That means what we really want is just this:
if (clickedElement.className === 'checkbox') {
todos[indexNumber].isComplete = clickedElement.checked
}
Much easier to wrap your mind around, eh? :smile:
I know I bring this up a lot, but it really makes code so much easier to read, especially weeks later or when someone who didn't write the code is looking at it. :wink: Humans are pattern-matching machines and when we notice a pattern, then a deviation from that pattern, it really stands out and becomes distracting - especially the more you write code.
A few examples of inconsistent indentation:
I noticed you assigned clickedElement.dataset.index
to clickedIndex
so that you could reuse it, which is fantastic! :tada: But... you didn't use it everywhere. Let's fix that. Reusing that value you save in the variable makes your program faster and easier to read.
Was there anything else you wanted to ask about regarding this lesson or JavaScript objects in general? Anything you're still confused about?
No, it makes sense. I might talk through the logic with you on Monday, but overall the division of the code into Global State/Elements/Events makes intuitive sense. I updated the code to use var indexNumber in the one other location with clickedElement.dataset.index. The indentation is fixed as well. New code has been pushed, and the new URL is http://skinny-cord.surge.sh/
Looks great! :shipit:
Hey @chrisvfritz, can you take a look at this? It's hosted here and meets the following project criteria:
title
,description
, andisComplete
propertiesform
that collects both a title and a longer descriptioncheckbox
, which will be checked ifisComplete
istrue
label
, which contains thetitle
of the tododescription
of each todo is stored in thetitle
attribute of thelabel
, so that it appears in a tooltip when users hold their mouse over the todoisComplete
begins asfalse
isComplete
attribute totrue
orfalse
, respectively