chenfan9 / MI-449-SS17-741-js-collect-and-display-information-kPt9Fl

0 stars 0 forks source link

Project Feedback #1

Open chenfan9 opened 7 years ago

chenfan9 commented 7 years ago

@KatieMFritz Can you take a look at this? It's hosted here and meets the following criteria:

chenfan9 commented 7 years ago

js project

KatieMFritz commented 7 years ago

Nice work, Fan! :fire: Your app works exactly as it should. 😁

You also did a wonderful job with naming your variables. It's easy to understand what each variable is, and you nailed the camelCase. 🐫 ✨

Reduce duplicate code

When I look at your updatePreview function and your updateHtml function, I see that they are almost identical. The only real difference is between previewParagraph.innerHTML and htmlParagraph.textContent.

Part 1: Use variables to reduce duplicate strings

Since you're assigning the exact same HTML string twice (the actual profile code, from <h1> to .</p>) , what would you think about assigning it to a variable first, then reusing it when you update the page? So for example, if I'm doing this:

element1.textContent = 'hello' + yourName
element2.innerHTML = 'hello' + yourName

Then whenever I want to update 'hello' + yourName, I have to do it in two places. We're not only too lazy for that 😄, but it can also cause bugs (especially if 'hello' + yourName is a longer, more complex string). If we update one string and forget to update the other the same way, then we have an inconsistency. A solution for this would be to move 'hello' + yourName to a variable, like this:

var greeting = 'hello' + yourName

element1.textContent = greeting
element2.innerHTML = greeting

Now whenever we want to update the greeting, we can do it in just one place. Future development is now much easier and it's more difficult to create bugs. Does that make sense?

Part 2: Combine functions

Since updatePreview and updateHtml use all the same variables (including your new one with the profile content) and should always happen at the same time (when a user types anything), you can combine them into one function that does 2 things.

In our greeting example above, instead of doing something like this:

var greetUser1 = function() {
  var greeting = 'hello' + yourName

  element1.textContent = greeting
}

var greetUser2 = function() {
  var greeting = 'hello' + yourName

  element2.innerHTML = greeting
}

we could do this:

var greetUser = function() {
  var greeting = 'hello' + yourName

  element1.textContent = greeting
  element2.innerHTML = greeting
}

This way, if you want to change the greeting variable, you only have to do it once, and also you only have to have one set of event listeners.


Does that make sense? Feel free to message me on Slack if you have any questions or would like a different example!

Otherwise, make your changes and comment back here when you're done. 😸

chenfan9 commented 7 years ago

updated

KatieMFritz commented 7 years ago

That looks AWESOME. Great job improving this over several rounds! 🔥 💥 🌴 😁

:shipit: