Roast my JavaScript please

Roast my JavaScript please
0.0 0

#1

Hello campers!

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?

codepen:


#2

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:

  1. Remove all the comments.
  2. Your AJAX object isn’t optimally configured. Remove async: false and dataType: jsonp.
  3. 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.
  4. 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.
  5. 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.
  6. 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.
  7. 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):


#3

One suggestion I would offer, when writing HTML markup in JS you can save yourself a lot of headaches if you use Template literals, for example:

        $("#resultBoxMain").append(
          `<div class="resultBox">
             <h1>${streamerName}</h1>
             <p>online playing: ${streamStatus}</p>
             <a target="_blank" href="${streamerLink}">
               <button class="onlineBtn">Stream</button>
             </a>
           </div>`
        );

#4

@ 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 :rofl:, I guess I have some reading to do!

Thanks so much for this response. I really really appreciate it.


#5

Heya @blogscot! Thanks for the response!

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?

Thanks for giving me this light bulb moment :slight_smile:


#6

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.

You can also kick back and do some watching:


Can't pass an input value into a javascript variable
#7

@PortableStick

You are a god. Thank you so much!

I have to run off to work right now, but I can’t wait to implement the userTemplate. It’s sooooo much cleaner!

Again, thank you very much! This has been so helpful!