chrisvfritz
commented
7 years ago Great work here! 🎉 🎉 🎉
Responding to comments
I added a new property to store the jokes like you showed me in the previous module. The only thing I did not like about my solution is any reference to the variable now has to load from local memory every time. I'm sure the difference is minuscule, especially in a small script like this, but in terms of clock cycles, a cache reference compared to a ram access is in the order of 3x less efficient. Which in the case of a event driven program, you are waiting on input, so it's practically 0x and doesn't really matter. I guess the way to solve it in this case would be to store it in a temp variable.
That assessment is 💯% correct! But while we're on the topic, there's actually a pair of utilities you can use for simple benchmarking in JavaScript: console.time and console.timeEnd. For example, to compare a cached vs non-cached version of your app:
jokes.stored = {
foo: {
setup: 'baoiear stoiear stoien arstoien aioers tr',
punchline: 'baoiear stoia rstoiean rstoien arosietz'
}
}
console.time('reads without cache')
for (var i = 0; i < 10000; i++) {
jokes.stored
}
console.timeEnd('reads without cache')
var cachedJokes
console.time('reads with cache')
for (var i = 0; i < 10000; i++) {
if (cachedJokes) {
cachedJokes
} else {
cachedJokes = jokes.stored
cachedJokes
}
}
console.timeEnd('reads with cache')In the console, you'd then see something like this:
reads without cache: 80.077ms
reads with cache: 0.597msThose are actual times I just measured. The non-cached version is much slower in this extreme case of fetching jokes 10,000 times in a row. And what's worse, operations with localStorage are actually blocking, meaning the browser window will just freeze up while it's working. Even at 80ms though, this would probably still be tolerable performance with that many fetches.
Variation between JavaScript engines
Due to the way JavaScript engines work, these times are also likely to vary a fair amount - probably between 40ms and 150ms in this case, though it should remain less than 1ms for the cached version. Different engines also optimize in different ways, so you'll get different results depending on your browser.
Why use a for loop in benchmarks?
There are a lot of different ways to perform loops in JavaScript. For example, recursive functions and Array.prototype.forEach, which I usually recommend using in application code, since it reads nicer to most people. All of these have different performance characteristics though and also different levels of performance stability.
While for loops aren't the nicest to look at, they're exceptionally low overhead and their performance is more stable than many other looping methods, so I favor them in performance-critical scenarios like this.
Testing different edge cases
In the case of our app, 10,000 reads in a row is unlikely to ever happen - but if it gets really complex, maybe we could get to 100 at a time? Since users input their own jokes however, it could be more difficult to predict how many they might want to save. A pretty extreme case might be 5000 jokes, so let's load up our app with 5000 jokes:
var testJokes = {}
for (var i = 0; i < 5000; i++) {
testJokes[Math.random().toString()] = {
setup: 'baoiear stoiear stoien arstoien aioers tr',
punchline: 'baoiear stoia rstoiean rstoien arosietz'
}
}
jokes.stored = testJokesThen run the same test for 100 reads in a row:
reads without cache: 1656.112ms
reads with cache: 11.963msOuch - now we're getting into dangerous territory. At this point, rendering the UI would still be an even bigger bottleneck, not to mention a usability nightmare the way it's currently designed. So much scrolling. 😣
When to write a benchmark
I think in cases like this, it's definitely worth doing a very quick benchmark if you're not familiar with the performance of a particular feature. If you find some low-hanging fruit that's easy to implement, then I usually go for the optimization. In this particular app, there's only one way new data is added or removed, respectively, so invalidating/updating the cache reliably wouldn't be too hard.
Application code vs library code
While we're talking about performance and other edge cases, the times I pay the most attention to these are in library code (rather than application code), where you're building a tool that will be used on many different projects and it's much more difficult to predict exactly how.
Since we're focused more on applications in our projects, I just put together a more generic localStorage adapter to give you a taste of some differences when working on library code. I added a bunch of comments, but if you have any questions, please don't hesitate. I hope it's helpful! 🙂
The defined property does not reference the object. The property instead calls the object copy constructor which would make the solution feasible.
Yep, right again! Nice observation. 😄
Is it ok if I use code comments to explain some design decisions or should I only comment on the functionality.
Great question. I personally don't mind - as long as it doesn't get so cluttered that it's difficult to read the program.
Project feedback
Now for some areas for improvement:
Complex getter for jokes.stored
I think this getter might be a little overcomplicated:
var jokes = window.localStorage.getItem('jokes')
try {
jokes = JSON.parse(jokes) || {}
if (typeof (jokes) !== 'object') {
throw new JokeException('Invalid Object')
}
} catch (e) {
// set local storage with empty object on failed parse Exception
// or Invalid Object Exception
jokes = {}
window.localStorage.setItem('jokes', JSON.stringify(jokes))
}
return jokes- We include two empty object fallbacks, first checking for a falsy value and then checking for a non-object. Simply checking for a non-object covers both use cases though.
typeofis kind of a weird operator, in that it's followed by its operand, so the parentheses intypeof (jokes)is actually unnecessary, just as they would be in(2) + (5).- In general, custom exception objects (like
JokeException) aren't very useful if you're just going to be throwing the exception yourself and then immediately catching it. I'd simply set jokes to the fallback. - I don't see any reason to save to
localStoragein the case where it contains a non-object. In fact, I think it could be useful for debugging to be able to see what value is being saved there.
Altogether, these optimizations would shorten the code to just:
var jokes = JSON.parse(window.localStorage.getItem('jokes'))
if (typeof jokes !== 'object') {
jokes = {}
}
return jokesUpgrading the generic adapter
Try incorporating my generic adapter into your code, using it as you might a library, with the generic code in a separate file loaded before your app. After that, I want you to upgrade it. 🙂
1. Checking for a valid object on writes
With localStorage data privately scoped in the generic adapter, the only way that we're ever likely to get a non-object is when the user writes to a resource. Right now though, there's zero validation. To fix this, create a new isObject helper function that we can use in the setter to if the new value we're given is really an object, then abort with a custom error if it's not.
Note that there's a little gotcha with typeof myVar !== 'object': it will also return true for arrays, since arrays are really a subtype of objects. There are a few different ways to check that this object is a plain object, not an array, but I'll let you do some research. 😄
2. Resetting data for all resources
For the generic adapter, let's say we found many users have a need to reset all the saved resources for an app. They want to be able to run data.$reset(), then any initialized resources will be removed from localStorage and the cache, with no trace of their existence.
Once you've built this into the adapter, use this upgrade to add a new Reset app button in the jokes app.
Note about in operator
This is not something you were expected to know and I'm not sharing this extra information with most students, but a quick note about the in operator, like used here:
requestedJokeKey in jokes.storedThat also looks at properties inherited from the prototype chain, meaning this:
'toString' in {}Will return true. So in your app, if you look for a toString joke, it should actually display:
undefined
undefinedAn alternative, which will only check for locally defined properties, is the hasOwnProperty method:
jokes.stored.hasOwnProperty(requestedJokeKey)Updating the entire UI when data changes
Right now, you're just calling updateJokesMenu when adding or removing a joke. That means if I request a joke called foo, realize it's not there and add it, I'll have to update my requested joke to something else, then change it back to foo in order to see the new joke that was just created.
Try using the updatePage function instead. One of the advantages is that we don't have to think about where inconsistencies might arise - we can just call it and guarantee that the UI will be in sync with the latest data. With the amount of data we're dealing with in our app, I think the very slight performance hit is probably worth the increased predictability.
Clearing inputs after submitting data
After adding or removing a joke, it's typically best practice to clear the associated inputs, as it's unlikely they'll want to submit with the same data twice.
I kept smiling looking at your code. 🙂 Keep up the great work here! I can see you pushing yourself and trying new things, which the exact right strategy for growing your skills to the max. 💥 ⚡️ 💥
austin1244
egillespie
@chrisvfritz Can you take a look at this? It's hosted here and meets the following criteria:
requestedJokeInputhas a value matching the key of an existing joke, thejokeBoxHTML is updated in this formatrequestedJokeInputdoes not match an existing joke, thejokeBoxcontent is updated toNo matching joke found.Remember this joke!button is clicked, a new joke is saved (or overwritten) in thejokesobject, using the data in the associatedinput/textareaelementsForget about it!button is clicked, the joke with that key (if any) is deleted from thejokesobjectjokesobject changes, the new version is saved inlocalStorageand used whenever the user visits the page againI added a new property to store the jokes like you showed me in the previous module. The only thing I did not like about my solution is any reference to the variable now has to load from local memory every time. I'm sure the difference is minuscule, especially in a small script like this, but in terms of clock cycles, a cache reference compared to a ram access is in the order of 3x less efficient. Which in the case of a event driven program, you are waiting on input, so it's practically 0x and doesn't really matter. I guess the way to solve it in this case would be to store it in a temp variable. Which is another thing I noticed. The defined property does not reference the object. The property instead calls the object copy constructor which would make the solution feasible. Anyways, probably just rambling at this point, but I thought i would explain some of the decisions.
Also, is it ok if I use code comments to explain some design decisions or should I only comment on the functionality.