Code Lab Feedback

[ntntnlstdnt]

Hey @chrisvfritz, can you take a look at this? It's hosted here and meets the following project criteria:

• [x] The current number of todo items is always displayed below the list of todos.
• [x] A "Clear all" button exists, which will clear all todos when clicked.
• [x] A "Can't decide!" button exists, which picks a random todo item from the user's list and shows it in an alert. (Hint: you may have to use Google here.)
• x Each todo item contains two additional buttons: "↑" and "↓", which move the item up or down in the list, respectively. If the item is already at the top or bottom, then the button to move it further up or down, respectively, should not appear. Again, Google will likely be helpful here.

[chrisvfritz]

Very nice! You're really on a JavaScript kick lately, eh? I just see a few areas for improvement:

↑ and ↓ buttons always appear

It looks like the up button still appears when an item is already at the top and the down button still appears when an item is all the way at the bottom. See if you can fix this.

Simpler way of moving items up and down

Adding the move method to arrays is clever, but there's actually a simpler solution in this case. To move something up, for example, you could do something like this:

var temp = todos[idx]
todos[idx] = todos[idx - 1]
todos[idx -1] = temp

This method uses a temporary variable to store a value so that it's not lost when overriding from the other variable - sort of like switching items from containers, one at a time, using another, empty container.

Unnecessary event delegation here

Since the "Clear All" and "Can't decide!" buttons are always present, you don't need to use event delegation in order to target them. In fact, since there's only one of each, it probably makes more sense to assign click handlers separately, directly to the elements by targeting them by an id. Does that make sense?

[ntntnlstdnt]

@chrisvfritz the deed is done!

[chrisvfritz]

Did you push up a new commit? I'm not seeing anything. :confused:

[ntntnlstdnt]

Sorry @chrisvfritz . Just pushed it

[chrisvfritz]

Excellent! After this refactor, there's just one more opportunity for simplification that I see. It looks like renderTodos() and numberOfTodos() are both called together... and, they're both render functions (functions that make changes to the page).

Since they're both render functions, I would follow the convention of prefixing render functions consistently (ex. starting with render). renderNumberOfTodos makes it very clear what the function does. numberOfTodos on the other hand, isn't as specific. I could imagine it rendering or just updating data somewhere.

Since these two functions are always called together, that also tells me that we also want a meta function that contains them both - something like this:

function renderAll() {
  renderTodos()
  renderNumberOfTodos()
}

What do you think? Does that make sense?

[ntntnlstdnt]

@chrisvfritz makes perfect sense. I also figure out a solution for the down arrow. I learned that reversing the array doesn't reverse the indices. Therefore, the previous version of the code didn't work as intended. Also, I found out that todos [idx + 1] didn't work because javascript concatenated the indices instead of adding them... So I guess I kinda hacked the system and voila! it worked! :+1:

[chrisvfritz]

Yay! :shipit: This isn't something I'll make you change now, but I also just noticed that this line is unnecessary, since getting a random todo doesn't actually change what's in the todos array. :smiley: