glomstea / MI-449-SS17-741-js-localstorage-wMUDce

0 stars 0 forks source link

Project Feedback #1

Open glomstea opened 7 years ago

glomstea commented 7 years ago

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

egillespie commented 7 years ago

Works great! 💃 I have a couple of suggestions to tighten up the code a bit:

Use if/else when possible

There's a spot where you have two if statements back-to-back that are mutually exclusive:

if (changeVariable === 1) {
  document.body.setAttribute('class', 'dark')
}
if (changeVariable === -1) {
  document.body.setAttribute('class', 'light')
}

You can turn this into an if/else to avoid unnecessary comparisons (and less overall code):

if (changeVariable === 1) {
  document.body.setAttribute('class', 'dark')
} else {
  document.body.setAttribute('class', 'light')
}

Shorter ways to multiply by -1

You have a very explicit multiplication by -1 in your code:

changeVariable = changeVariable * -1

That can be shortened up in two different ways:

changeVariable = -changeVariable
changeVariable *= -1

The first line is a shorter way of multiplying changeVariable by -1. The second line is a short way to multiply a value by a number and reassign the result to itself.

They're all fine, but I thought you may be interested in trying one of them out. (i.e. you don't have to change your multiplication lines if you don't want to)


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! 🎸

glomstea commented 7 years ago

@egillespie Fixed what you were asking for.

egillespie commented 7 years ago

Very nice! :shipit: