Project Feedback

[Storingo]

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

• [x] Includes a button that when clicked, will toggle the website between day and night themes
• [x] When a user changes the theme, their choice is remembered in localStorage
• [x] The class on the body element is always up-to-date with the current theme (either day-theme or night-theme) -- you can use document.body.setAttribute('class', 'some-class-name') to set the class on the body
• [x] When the body has the class day-theme, the background of the website is light-colored, with a darker color for the text
• [x] When the body has the class night-theme, the background of the website is a very dark color, with a lighter color for the text
• [x] Somewhere on the page, the user is told how many times they've visited the website (starting with 1 the first time)

[chrisvfritz]

Hmm, I'm getting a 404 at the hosted URL. Did you forget to deploy there or is Surge having some issues?

[Storingo]

Okay I believe I fixed it. I must have miss-typed something the first time.

[chrisvfritz]

I'm still getting the 404 unfortunately. 😕 I'm using this link:

https://mi-449-ss17-740-js-localstorage-wmudce-storingo.surge.sh/

[Storingo]

OK this time it should work. I redid surge and clicked the link in your last comment and it worked. I was very slow and extra careful typing it in this time.

[chrisvfritz]

Looking much better now! 👍 A few areas for improvement I see:

Using fallback values for cleaner code

This code works and is very readable:

if (visits === null) {
  visits = 1
} else {
  visits = parseInt(visits)
  visits = visits + 1
}

But it could actually be simplified slightly. In this case, any falsy value (e.g. NaN, 0) is something we wouldn't mind replacing. So using the "or" operator (||) from the previous lesson, we could simplify the code to:

visits = parseInt(visits) || 0
visits = visits + 1

Or, if you wanted to be really fancy, just:

visits = (parseInt(visits) || 0) + 1

A similar simplification could be made with the time variable:

var time = window.localStorage.getItem('timeOfDay')

if (time === null) {
  document.body.setAttribute('class', 'day-theme')
} else {
  document.body.setAttribute('class', time + '-theme')
}

Since again, we'd want to replace any possible falsy values (e.g. null, '') with 'day', we can simplify with || to:

var time = window.localStorage.getItem('timeOfDay') || 'day'
document.body.setAttribute('class', time + '-theme')

In this case, since there are actually multiple places where we'd want to update this class, I recommend putting the code above into an updateTheme (or similar) function.

Simplifying if statements

There's an if statement here that's a little overly complex:

if (timeOfDay === null) {
  timeOfDay = 'night'
} else if (timeOfDay === 'day') {
  timeOfDay = 'night'
} else {
  timeOfDay = 'day'
}

In this case, there are only 3 possible states: null, 'night', and 'day'. We only want to set the timeOfDay to 'day' when it's currently 'night'. Otherwise, we always set to 'night'. That means this simpler if statement will do the same work:

if (timeOfDay === 'night') {
  timeOfDay = 'day'
} else {
  timeOfDay = 'night'
}

time vs timeOfDay

I noticed there are two variables that store the same information here. Why not combine them into a single variable that is always kept up-to-date?

BONUS: Shorthand for a = a + x

This isn't something to be "fixed", but when you're adding something to a variable, like this:

visits = visits + 1

There's actually a shorthand in JavaScript (+=) that does the same thing:

visits += 1

As an aside, this also works for other math operators:

visits -= 1 // visits = visits - 1
visits /= 2 // visits = visits / 2
visits *= 3 // visits = visits * 3

[Storingo]

Thanks for the feedback, the code looks a lot cleaner using these simplifications I will definitely keep them in mind for future work. I have updated github and surge.

[chrisvfritz]

Great work! 🎉 :shipit: How did you feel about this project? Are there any lingering questions on your mind that I can answer?

[Storingo]

Nope all good here but thank you!