4
\$\begingroup\$

I've written a piece of code which utilizes a for loop to iterate this array in JavaScript. This is my first attempt and I wonder if there is an optimal way to do this.

Fiddle

JavaScript

var gifCollection = {
"gifs": [
  {
  "gifName" : "gif-One",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Two",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Three",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
  {
  "gifName" : "gif-Four",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Five",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Six",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Seven",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Eight",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Nine",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
 },
 {
  "gifName" : "gif-Ten",
  "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
  }
 ]
};

var gifLink = gifCollection.gifs;
var div = document.getElementById('#vids');

for (var i in gifLink) {
  var src = gifLink[i].gifLink;
  //alert(gifLink.gifLink);
  console.log(src);
  //div.innerHTML = div.innerHTML + '<img src="' + src + '" />';
  $("#vids").append('<div id="wid-3"><img src="' + src + '"/></div>');
}

HTML

<div class="container">
 <div class="video">
  <div id="vids" class="row">
  </div>
 </div>
</div>
\$\endgroup\$
  • 1
    \$\begingroup\$ I guess those are too few lines to write a proper review for. (The object-setup not counted, of course). But $("#vids").append($('<div>').attr('id', 'wid-3').append($('<img>').attr('src', src))); might be a little bit cleaner code, not necessarily better though. \$\endgroup\$ – tkausl Jul 19 '15 at 2:36
  • \$\begingroup\$ @tkausl - thank you for the insight...cleaner code is always something i'm trying to learn and this definitely helps! \$\endgroup\$ – user2647510 Jul 19 '15 at 2:52
5
\$\begingroup\$

There aren't many things wrong. And the code itself is surprisingly short. But this will be a really long review.

The first thing that bothers me is that you talk about JSON, but you show an object. That's fine, but you should show how you decode the JSON into that object. But, within the scope of this review, I'll refer to it as an object, which is what was presented. Not to mention that all the links point to .jpgs.

Now, lets tackle the code!

Javascript:

The indentation of that object is really off. Not to mention that you have mixed quote styles.


Each gif has the following:

{
    "gifName" : "gif-One",
    "gifLink" : "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg"
}

We know those are gifs, so, why repeat gif in every single property? Using just name and link is enough.

And you don't even use the gifName anywhere! So, just drop it! And just to have there to say it is gifs, you can remove it and just use an array.

The whole object can be something like this:

var gifCollection = [
    'http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg',
    'http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg',
    ' ... '
];

Notice that you have to use " instead if you are using this in JSON! As I refered earlier, I would review it as an object.


Looking a little below, you have this line:

var div = document.getElementById('#vids');

Well, that won't work. What you have there is a CSS selector. Fear not, just remove the #. But a few lines below you have this:

//div.innerHTML = div.innerHTML + '<img src="' + src + '" />';

It seems like you made an attempt, but it didn't worked. Since these 2 lines aren't doing anything, just remove them.

Anyway, you are using jQuery below!


Right after it, you have this block:

for (var i in gifLink) {
    var src = gifLink[i].gifLink;
    //alert(gifLink.gifLink);
    console.log(src);
    //div.innerHTML = div.innerHTML + '<img src="' + src + '" />';
    $("#vids").append('<div id="wid-3"><img src="' + src + '"/></div>');
}

These comments contain old code. Just remove them! They won't kill you. And that console.log can go away as well, since it isn't useful to make the code work. It may be in debug.

But wait! You have this line:

$("#vids").append('<div id="wid-3"><img src="' + src + '"/></div>');

Oh no! You are creating duplicated ids! They MUST be unique! Or that will be invalid HTML. Use a class instead. Also, don't be afraid of using the id videos. No one will die over 2 bytes. And it is way more clean.

Since I've suggested to change the structure of your object, you have to use a different for loop. Applying the changes above explained, and fixing the quotes:

for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    $('#videos').append('<div class="video"><img src="' + gif + '"/></div>');
}

I've kept the length there because it will speed up your code quite a lot. Speaking in 'speeding up', you are re-running $('#vids') per gif. Just store it into a variable. Like this:

var videos = $('#videos');
for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    videos.append('<div class="video"><img src="' + gif + '"/></div>');
}

Now you won't be running jQuery everytime and it will be way faster!


Putting your Javascript code all together:

var gifCollection = [
    "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg",
    "http://buffalogrove.sat.iit.edu/thumb/turkey_karadeniz_region-t2.jpg",
    " ... "
];

var videos = $('#videos');
for (var i = 0, length = gifCollection.length; i < length; i++) {
    var gif = gifCollection[i];
    videos.append('<div class="video"><img src="' + gif + '"/></div>');
}

HTML:

The HTML is pretty basic, but there's still plenty to improve. Lets look at it on it's original state:

<div class="container">
    <div class="video">
        <div id="vids" class="row">
        </div>
    </div>
</div>

Alright, you have a .container which has a .video which has #vids inside? What? Videos inside a video?

What I would do:

  1. Remove .video. It's useless there.
  2. Change the id to videos
  3. Give the class row to the container.
    The container is the one that will control the width of your #videos.
    It makes sense to set it this way.
  4. Closing videos in the same line
    Since you are using jQuery, later on you may want to use the pseudo-selector :empty, which won't work if you have newlines in it.

Applying all the changes:

<div class="container row">
    <div id="videos"></div>
</div>

And yes, it works just like the old HTML.

\$\endgroup\$
  • \$\begingroup\$ your answer is superb! I'm still learning and as a result made some errors, but with your answer and more importantly your explanation - i have a clearer understanding. However, I do have a question regarding the for loop you suggested in the answer. If my Object changes to include different keys, like 'Name' and 'Link', and its corresponding values, would the for loop you suggested still be valid if I want to pull the corresponding values? My assumption is that you'd have to declare variables with the corresponding keys: var src = link[i].link; and var title = name[i].name; \$\endgroup\$ – user2647510 Jul 20 '15 at 0:14
  • \$\begingroup\$ @user2647510 I'm really glad it helped you. Regarding your comment, using the for loop in that exact way, and with the link and name in the object, you could just do var gif = gifCollection[i]; console.log( [ gif.name, gif.link ] ). You don't need a new variable for each property in the object. If I understood you incorrectly, please, just tell me. \$\endgroup\$ – Ismael Miguel Jul 20 '15 at 0:19
  • \$\begingroup\$ thats what i was looking for. But just to make sure i'm understanding the concept, the reason we can use gif.name and gif.link is because the variable var gif holds the object gifCollection with the names(keys - same concept, right?) of name and link. var gif = gifCollection [{ 'name' : 'First Gif', 'gifLink' : 'http://site.com'},{ etc }]. Thanks again! \$\endgroup\$ – user2647510 Jul 20 '15 at 1:07
  • \$\begingroup\$ I updated my fiddle with the code you supplied, but couldnt get it to work with the for loop you suggested, specifically the length. This is what worked for me: for (var i = 0; i < dataCollection.videoData.length; i++) versus for (var i = 0, length = gifCollection.length; i < length; i++) can you explain the difference...Many thanks! \$\endgroup\$ – user2647510 Jul 20 '15 at 1:50
  • \$\begingroup\$ @user2647510 The difference is that you now have an object with the key videoData that contains an array. That array is the one with the length. That's why gifCollection.length doesn't work. \$\endgroup\$ – Ismael Miguel Jul 20 '15 at 8:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.