Open austin1244 opened 7 years ago
I like what I see! 🎈
A few areas for improvement:
title
elementIt's technically invalid HTML when it's empty, so let's add something for the sake of maintaining best practices. 🙂
link
elements in the head
It's best practice to put link
elements after <meta charset="utf-8">
and the title
element, especially since the browser will often use that character set to parse resources the link
element points to.
name="themeButton"
instead of id="themeButton"
?This isn't something that necessarily has to change, depending on the reasoning. I saw this comment:
// Only applies to one button, so I made it anonymous
But I wasn't sure exactly what you meant.
charset
on script
elementsIf the page is already specifying a charset
, there's actually no need to specify the same character set on linked resources, as the browser will use that character set by default. See this from the HTML spec:
To sum up, conforming user agents must observe the following priorities when determining a document's character encoding (from highest priority to lowest):
- An HTTP "charset" parameter in a "Content-Type" field.
- A META declaration with "http-equiv" set to "Content-Type" and a value set for "charset".
- The charset attribute set on an element that designates an external resource.
textContent
vs innerHTML
It's usually best to use textContent
rather than innerHTML
when you don't need the content to be parsed as HTML, as it provides a small (I believe about 20%) speed boost on very short strings and a larger boost as strings grow longer. It's a trivial difference here, but is good to know as you work on more complex applications.
Ternaries are useful when you need a conditional to be an expression. In most other cases though, I would avoid them. Even though they're shorter to write, they generally take longer to read and can cause some confusion in some cases. For example, in this line:
page.theme === null ? page.theme = theme.light : page.theme
It's actually doing the same work as:
if (page.theme === null) {
page.theme = theme.light
} else {
page.theme
}
Looking at the longer if
statement, it's much clearer that the else
isn't actually doing anything. So it could be shortened to:
if (page.theme === null) {
page.theme = theme.light
}
This is two lines longer, but it's much easier to tell what's going on because it reads like normal English:
if page.theme is equal to null, then page.theme is assigned theme.light
Looking back at this line:
page.theme === null ? page.theme = theme.light : page.theme
We don't even know it's a conditional until we see: ?
. Then we have to search for :
(an even tinier character) to tell where expressions start and end. Which brings me to...
In some cases (especially with assignment), ternaries being an expression can be really useful, like in this example:
currentTheme = currentTheme === theme.dark ? theme.light : theme.dark
However, because of the aforementioned readability issues of ternaries, I'd actually recommend spreading them over multiple lines in most cases:
currentTheme = currentTheme === theme.dark
? theme.light
: theme.dark
Then it's much clearer with a quick scan that you're using a ternary and where the expressions are separated.
I personally feel very short ternaries can still be one line though. For example:
var number = isTruthy ? 1 : 0
Since there are many fewer characters, it's easier not to miss the vital ternary operators.
These ternaries would actually not be necessary at all, if we included fallback values for the assignment, with the ||
operator:
page.visits = parseInt(window.localStorage.getItem(storage.visits)) || page.visits
page.theme = window.localStorage.getItem(storage.theme) || page.theme
Then if we also updated the default value for visits
to 0
:
var page = { visits: 0, theme: theme.light }
We could replace both of these lines:
page.theme === null ? page.theme = theme.light : page.theme
isNaN(page.visits) ? page.visits = 1 : page.visits++
With just:
page.visits++
This works because in both cases, there aren't any valid falsy values we could pull from localStorage
.
You're working at a great pace in the course, so is there anything else you'd like to dive into? If you're interested, I'd be happy to show you refactoring examples of useful programming patterns that can help with these kinds of problems, such as the:
storage.visits
like a normal property, but with special powers that would automatically keep it in sync with localStorage
). Besides convenience, another advantage is that it becomes easier to implement type checks or validations if you wanted. And if you decided to save to another storage mechanism, you'd only have to touch your data adapter code - the main application code could remain exactly the same.Or if there are any other skills on your mind that would help you level up, I'm happy to go down completely different directions. Just let me know. 🙂
@chrisvfritz
title
elementlink
elements in the head
name="themeButton"
instead of id="themeButton"
?I used the name because It came up in atom when I typed button \n I consulted with w3 and concluded that is mainly used for form data, so its now set with the id attr
// Only applies to one button, so I made it anonymous
But I wasn't sure exactly what you meant.
This I was refrencing to the anomymos function for the event listener. I didn't feel like its was neccessary to make it a named function since there was only one button that needed that functionality.
charset
on script
elementsThe charset was generated by atom, but I will keep that in mind next time.
textContent
vs innerHTML
I agree I got a little carried away with the ternary operators. I would chalk that up to unnecessary refactoring.
Also, I liked the setting visits to 0 and incremented no matter what. That was a cleaner way to handle page visits. Which I will add, I appreciate you taking the time to do a indepth analysis.
I left the:
if (page.visits !== null) {
window.localStorage.setItem(storage.visits, page.visits)
}
because it was from the lesson to guarantee no null values are saved.
You're working at a great pace in the course, so is there anything else you'd like to dive into?
Nothing particular, I am more comfortable on the server side from raw experience, I work part time as a student back end software dev at Matrix, so I am curious about that side of the class. I also prefer the OOP paradigm over procedural, which has better use on the server.
Data adapter pattern: I am familiar with this pattern from a CSE class if you are referring to this? https://en.wikipedia.org/wiki/Adapter_pattern
View model pattern: I am familiar with this pattern from a back end perspective (eg. cakephp). I guess if backbone js falls into that category than vaguely with that as well.
Looks great! 🎉 :shipit:
This I was refrencing to the anomymos function for the event listener. I didn't feel like its was neccessary to make it a named function since there was only one button that needed that functionality.
Got it. Thanks for clarifying. 🙂
used the name because It came up in atom when I typed button
The charset was generated by atom
Atom's built-in snippets are often helpful, but they do sometimes have strange defaults. I'm looking forward to https://github.com/atom/snippets/issues/55 being fixed, so that we can selectively disable built-in snippets that don't make sense!
I looked up some other languages that have fallback values and I wasn't a fan of how they were implemented. JS has a nice implementation for this, but I believe it's because JS is loosely typed.
I agree! 👍 In strongly typed languages, I often like the Maybe/Option type, which can handle null values, but also typically forces you to handle them specifically. I'm not a C# dev, but here's an example implementation for C#, since you might have worked with that at Matrix. Pattern matching is another language feature that can serve this purpose well - particularly in more functional languages, where pattern matching tends to be more robust.
Nothing particular, I am more comfortable on the server side from raw experience, I work part time as a student back end software dev at Matrix, so I am curious about that side of the class.
Excellent! We'll start server-side lessons this coming week. 🙂
I also prefer the OOP paradigm over procedural, which has better use on the server.
As we dive deeper into JavaScript and server-side, the style we'll be focusing on will be more functional than OOP or procedural. I don't mean that in a pretentious way - like it actually works better 😂 - it's just a bad name for a particular style of programming that tends to favor more declarative rather than imperative code.
Hopefully that will be an interesting new perspective for you!
Data adapter pattern: I am familiar with this pattern from a CSE class if you are referring to this? https://en.wikipedia.org/wiki/Adapter_pattern
View model pattern: I am familiar with this pattern from a back end perspective (eg. cakephp). I guess if backbone js falls into that category than vaguely with that as well.
Excellent! In this case, I'll show you a really cool feature of JavaScript that can help in implementing adapter patterns in general: Object.defineProperty
. Among other things, it allows you to proxy the getters and setters for properties, which can provide a simple interface to abstract away where your data is actually coming from.
Here's an example with localStorage
:
var data = {}
Object.defineProperty(data, 'visits', {
get: function () {
var rawVisits = window.localStorage.getItem('visits')
return parseInt(rawVisits) || 0
},
set: function (newValue) {
window.localStorage.setItem('visits', newValue)
return newValue
}
})
Object.defineProperty(data, 'theme', {
get: function () {
return window.localStorage.getItem('theme') || 'day'
},
set: function (newValue) {
window.localStorage.setItem('theme', newValue)
return newValue
}
})
Now we have a data
object with two properties: visits
and theme
. We can access these properties like any other. For example:
data.visits // returns a number from localStorage
data.visits++ // gets the current number from `localStorage`, increments it, then saves it
Some advantages:
theme
is set, its value is one of a limited set of valid themes (e.g. 'day'
, 'night'
).Limitations:
push
(to add a new item to an array) or the delete
keyword (to remove a property from an object) don't actually call set
, since they're not replacing the existing value, just modifying it. Methods can be proxied as well using other strategies, but keywords like delete
would simply have to be avoided. When Proxy is finally well-supported, it can solve many problems... but not until Internet Explorer usage drops to almost 0.Object.defineProperty
, you either need to register any properties you use upfront or create a utility that registers new properties for you (e.g. data.$define('new-property', newValue, type)
.I hope you found that useful! 🙂 Let me know if you have any questions/thoughts.
@chrisvfritz Can you take a look at this? It's hosted here and meets the following criteria:
localStorage
body
element is always up-to-date with the current theme (eitherday-theme
ornight-theme
) -- you can usedocument.body.setAttribute('class', 'some-class-name')
to set the class on thebody
day-theme
, the background of the website is light-colored, with a darker color for the textnight-theme
, the background of the website is a very dark color, with a lighter color for the text1
the first time)