chrisvfritz
commented
8 years ago This all works, which is the most important thing!
Slight over-engineering / over-thinking
All of this code works, but some of the logic is a tiny bit redundant. For example, let's take this code:
if (viewSelect === 'dayMode') {
localStorage.setItem('viewMode', 'nightMode');
}
else if (viewSelect === 'nightMode'){
localStorage.setItem('viewMode', 'dayMode');
}
else {
localStorage.setItem('viewMode', 'nightMode');
}The logic makes complete sense, but if we break it down, what we really want to do is set the viewMode to dayMode if the viewMode is nightMode. And in all other cases, set it to nightMode. Like this:
if (viewSelect === 'nightMode'){
localStorage.setItem('viewMode', 'dayMode');
}
else {
localStorage.setItem('viewMode', 'nightMode');
}The code does the same thing, but it's a bit less complex.
Another instance of overthinking the problem a bit can be seen in this code:
if (viewSelect === 'dayMode') {
document.body.className = 'dayModeClass';
}
else if (viewSelect === 'nightMode') {
document.body.className = 'nightModeClass';
}We can shorten it to:
document.body.className = viewSelect + 'Class';But something you're not currently handling is the very first page load, when viewSelect will be null. Fortunately, there's a trick to neatly handle that:
document.body.className = viewSelect ? viewSelect + 'Class' : nightModeClass;
That's a ternary and it's basically a very short way of writing if x then y else z. We're first checking if viewSelect has a value with viewSelect ?, then if it does, we assign document.body.className to viewSelect + 'Class'. Otherwise (:), we set it to nightModeClass.
Finally, whenever there are parts of strings that you see repeated a lot, it's very possible that you can strip out the repeated bits. For example, we can shorten dayMode and nightMode to just day and night, since we know they'll always be modes. And dayModeClass and nightModeClass can just be day and night as well.
Similarly with variable names, when there are prefixes sometimes, like with storedCount, that's typically a sign that count should also have a prefix - newCount might be appropriate in this case.
Don't use the onload attribute
One problem with <body onload="..."> is that the JavaScript only runs after the entire page is loaded and rendered. That means it'll even wait for images to load! The result is quite a noticeable delay before the JavaScript runs. Instead, what I encourage you to do for JavaScript you want to run immediately (and also at other times), is to call the function after you declare it, like this:
function myFunction() {
// some code...
}
myFunction() // This line immediately runs the function when this script is loadedUpdating our JavaScript
Using these tips, here's an updated version of your JavaScript. I've included comments to explain what I'm doing along the way.
function countVisits () {
storedCount = localStorage.getItem('count');
// Renamed `count` to `newCount` for more explicit naming and
// also using a ternary for a shorter if statement.
var newCount = storedCount === null ? 1 : parseInt(storedCount) + 1;
// Renamed `Counter` to `count`, as variable names should always
// begin with a lower case letter.
var counter = document.getElementById('visitCount');
counter.innerHTML = newCount;
localStorage.setItem('count', newCount);
}
// Calling `countVisits` right after I declare it so that it runs when the page loads
countVisits()
function updateView() {
var viewSelect = localStorage.getItem('viewMode');
// Setting the body class to `viewSelect` - or, if there is no value in `viewSelect` -
// then to the string `'day'`, which will be our default view mode.
document.body.className = viewSelect || 'day';
}
// Calling `updateView` right after I declare it so that it runs when the page loads
updateView()
function toggleView () {
// Getting the mode based on the body, again using a ternary for a shorter `if-else`.
var newMode = document.body.className === 'night' ? 'day' : 'night';
localStorage.setItem('viewMode', newMode);
// Updating the view instead of refreshing the page, because that messing with the
// visit counter and also is less instantaneous.
updateView()
}
document.getElementById('modeButton').onclick = function () {
// Just toggling the view when the button is clicked.
toggleView()
};Let me know what you think! If you have any questions about this code, please do let me know and I'll be happy to explain any aspect in more detail. :smiley:
zacharyweston
Hey @chrisvfritz, can you take a look at this? It's hosted here and meets the following project criteria:
document.body.className = 'some-class-name'