I am looking for criticism on my twitch JSON API challenge. Specifically I am looking for a critique on my JS code.
I understand that the styling could use some love. I was mainly focused on the JavaScript, and I am itching to hit the next section of algorithm challenges(css will have to take a backseat for now).
How should I go about cleaning up/refactoring my code?
How does my code differ from what you would have done? Why is your way better?
Do the coding gods approve? Should I sacrifice myself to them?
Alright. Keep in mind that youâve done a good job and you should totally consider this project complete. That said, hereâs some code review:
Remove all the comments.
Your AJAX object isnât optimally configured. Remove async: false and dataType: jsonp.
Writing to global variables is asking for trouble. onlineStreams, offlineStreams, and allStreams could be avoided by creating a function like addUser that would handle creating the markup for each user as it comes in from the AJAX response.
Lines like var streamerName= allStreams[i]._links.self.slice(37); are confusing. The 37 here is what we call a âmagic numberâ. Itâs important and specific, yet not immediately clear where it came from or why itâs used. I saw a comment farther up explaining what it is, but self-documenting code should be preferred. To this end, any weird numbers that you need to use should be stored in a variable with a descriptive name. In this case, something like pathLength might work.
Related to (3), all of the strings used for generating markup make the code a bit hard to read. Moving all of those into their own functions would help a lot. I almost always use Handlebars.
Itâs helpful when using jQuery to cache your DOM lookups in variables. $resultBoxMain = $('#resultBoxMain');, for example. Keep in mind that $() is a function, and every time you run it, jQuery performs a (somewhat) expensive operation to find the right element. Save that result to prevent unnecessary work.
I would use promises instead of callbacks, which could help clean up and reduce the code.
I canât think of much else. Projects that use AJAX to generate markup are complicated and difficult to organize, so you should feel good that itâs done. Hereâs a much simpler project that would serve as an example of almost everything I said above (there are a buttload of comments, but itâs meant to be a demonstration):
@ PortableStick Thank you so much for the detailed response! This is exactly what I was looking for. I do have a couple of follow up questions:
Could you elaborate on this a bit? I think I turned async off in an attempt to fix an issue I was having earlier on. I think I forgot about it as I kept on with the rest of the code, and since it didnât break anything I left it.
My question is: what exactly is suboptimal when I turn async off and set the dataType to jsonp? Are the two related to one another? All I know is that when calling some APIâs jsonp helps with some errors.
I havenât run into any issues when writing to global variables yet (atleast I think). Why is it asking for trouble?
Regarding the addUser function, would it be storing object properties into the dataset? could you show me an example?
I am not even sure what a promise is , I guess I have some reading to do!
Thanks so much for this response. I really really appreciate it.
Your code here looks infinitely better than mine. This is kind of one of those /facepalm moments, why did I no think to write it out to look like HTML?
Also when you say Template literals do you mean writing the HTML markup just like I would outside of JavaScript?
Turning async off makes the request synchronous, so the requests will stop the rest of your app from working until they are complete. JavaScript is single threaded, so we can only ever do one thing at a time in an app. However, the browserâs APIs allow us to send a task off to be completed by another thread, freeing the app up to do other things, like render the page or respond to user input. This is what happens when we make AJAX requests - we give the browser a url to contact and a callback function to run when the response comes back, but JS itself doesnât make the request. This is asynchronous JavaScript in a nutshell, and by turning async off youâre forcing your app to wait on the slowest part of your code. Ironically, this setting isnât even valid because you set the datatype to jsonp, which disables synchronous requests. So, might as well leave it at default.
Setting the datatype to âjsonpâ isnât necessary because the API youâre calling supports CORS. JSONP is a hack that web devs have had to use in order to get around the same origin policy (SOP). This is all really confusing, and I wouldnât be doing you any favors trying to explain it here. The short story is that SOP is a really good thing but it has made life difficult for developers needing to get data from a remote source. JSONP was only needed because we didnât have a formal way for data servers to decide who can get information remotely. Now that we have CORS, JSONP will go the way of the non-avian dinosaurs. Your app works after deleting both these options, so no sense in having them specified.
Imagine that more than one function depends on your global variables. Since all the functions are accessing the same data, if any one of them changes that data, then theyâre all going to be affected. Now imagine that you have asynchronous network calls which write this data - you donât know when these network calls will be finished, so they could write to your variables when youâre not expecting them to, way before you want them to, or not at all. Managing global variables can be like herding cats. In your case, you made good use of jQueryâs AJAX events and your app only runs when all of the network calls have finished. There are a few problems with this approach in bigger apps, however.
Youâd have to write a lot of code to handle errors. The API youâre using is simple and not error prone, but in bigger apps we have to worry about more than just the happy path.
Relying on global AJAX events can also be a lot slower than processing each piece of data as it comes in. Your app only has to wait on a few calls to finish, and theyâll all be successful in a reasonable amount of time, but modern apps can make hundreds of requests. Think about Netflix - each movie that you see is the result of a request (I think each row of movies is a single request, but not entirely sure). If Netflix waited until all of the data had come in before rendering, the app would seem a lot slower.
Your app makes one request when itâs loaded, but we often have to make multiple requests throughout the appâs life. If you wanted to allow users to search for Twitch users and add them to a list, working around the global AJAX events becomes a much bigger hassle than itâs worth.
The idea is not to store the data, but to render it as it comes in. Thereâs really no sense in holding onto it. Check out my Codepen I linked to. On line 14, I render each individual user:
$(".users").append(userCardTemplate(user));
userCardTemplate is a Handlebars function I created. All it does is take a piece of data and apply it to a template. Itâs really no different from what youâre doing on line 64 of your own app, just building markup in a string and then appending it to another element. @blogscotâs got a great example in his post using the template literals, but imagine that string being returned from a function:
function streamingUserTemplate(user) {
return `<div class="resultBox">
<h1>${user.streamerName}</h1>
<p>online playing: ${user.streamStatus}</p>
<a target="_blank" href="${userl.streamerLink}">
<button class="onlineBtn">Stream</button>
</a>
</div>`
}
// now look at how easy it is to render each user
for(let i=0;i<streamArray.length;i++){
var url = `https://wind-bow.glitch.me/twitch-api/streams/${streamArray[i]}`;
$.getJSON(url, function(response) {
if(status !== null) {
$resultBoxMain.append(streamingUserTemplate(response));
} else {
$resultBoxMain.append(offlineUserTemplate(response));
}
});
}
Now youâre not herding cats, you can handle errors more easily, and your app will be more performant.
, I guess I have some reading to do!
