Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm working on a 3 columns photo website (responsive), and I'm parsing the img src from a XML.

I need to prepend a HTML code every 3 elements so my photo wall respect my responsive class.

I just found a cheap solution, repeating the xml loop (for).

Do you have an advice to give me, to do it right, to improve my code ?

Thanks

This is my code :

$(document).ready(function(){
 xmldata=new Array();      //initialise array for XML data
 $.ajax({                      //get the XML data
  url: "xml/gallery.xml",             //URL to get the data from
 success: function(data) {         //if we succeed, set everything up. We don't expect    failure.
  $(data).find("image").each(function()
 {
  xmldata.push($(this));        //add item into array
 });


 totalNum = xmldata.length;   

 gallery_images();              

} 


 });   

});






 function gallery_images(){

 for (var i = 0; i <= 2; i++) {

 data=xmldata[i];          

 img_src=$(data).attr("src");          
 href=$(data).attr("href");
 title1=$(data).attr("title1");
 client=$(data).attr("client");





  var video_gallery_html = '';
  video_gallery_html += '<article class="col span_8"><div class="title_div span_24 ">';
  video_gallery_html += ' <a href="#"> <img src="images/linea.png" alt="line_divisor" /></a>';
  video_gallery_html += '  <h1 class="title_item col span_12 _left">DENIS ROVIRA</h1>';
  video_gallery_html += '   <h3 class="title_more col span_12 _right">See all projects</h3></div><!-- title_div -->';
  video_gallery_html += '   <a href="#"> <img src="'+ img_src +'" alt="image_item" /></a>';
  video_gallery_html += ' <p>Nescafe Gold</p></article><!-- span_8 -->';


   $(".videos_container").prepend(video_gallery_html);




 };


 for (var i = 3; i <= 5; i++) {

 data=xmldata[i];          

 img_src=$(data).attr("src");          
 href=$(data).attr("href");
 title1=$(data).attr("title1");
 client=$(data).attr("client");




 var video_gallery_html2 = '';
 video_gallery_html2 += '<article class="col span_8"><div class="title_div span_24 ">';
 video_gallery_html2 += ' <a href="#"> <img src="images/linea.png" alt="line_divisor" /></a>';
 video_gallery_html2 += '  <h1 class="title_item col span_12 _left">DENIS ROVIRA</h1>';
 video_gallery_html2 += '   <h3 class="title_more col span_12 _right">See all projects</h3></div><!-- title_div -->';
 video_gallery_html2 += '   <a href="#"> <img src="'+ img_src +'" alt="image_item" /></a>';
 video_gallery_html2 += ' <p>Nescafe Gold</p></article><!-- span_8 -->';


  $(".videos_container2").prepend(video_gallery_html2);




 };


 } 
share|improve this question

2 Answers 2

Perhaps using a template system can help you, check http://handlebarsjs.com/ you can integrate with jquery and reuse partial html code as a templates

<script id="entry-template" type="text/x-handlebars-template">
<div class="entry">
  <h1>{{title}}</h1>
  <div class="body">
    {{{body}}}
  </div>
</div>
</script>

<script type="text/javascript" charset="utf-8">
// Compiling template to generate function
var source   = $("#entry-template").html();
var template = Handlebars.compile(source);

//  When you receive your data content var just call this:
var context = {title: "My New Post", body: "This is my first post!"};
// in your case maybe it is:
// var context =    { img_src: $(data).attr("src"), href=$(data).attr("href"), title1: $(data).attr("title1"), client : $(data).attr("client") };
var html    = template(context);

</script>
share|improve this answer

First, you should avoid global variables. There is almost always other ways to use variables without making them global. Also, keep in mind that any variable that you do not declare with var is considered global.

Your code within the loops are almost exactly the same. Copy that into a function and call the function.

$(document).ready(function(){
    var xmldata=new Array();      //initialise array for XML data
    $.ajax({                      //get the XML data
        url: "xml/gallery.xml",             //URL to get the data from
        success: function(data) {         
            $(data).find("image").each(function() {
                xmldata.push($(this));        //add item into array
            });

            gallery_images(xmldata);              
        } 
    });   
});

function addImage(data, container) {

    var img_src=$(data).attr("src"),         
        href=$(data).attr("href"),
        title1=$(data).attr("title1"),
        client=$(data).attr("client"); // note that I am initializing all variables here.

        var video_gallery_html = '';
        video_gallery_html += '<article class="col span_8"><div class="title_div span_24 ">';
        video_gallery_html += ' <a href="#"> <img src="images/linea.png" alt="line_divisor" /></a>';
        video_gallery_html += '  <h1 class="title_item col span_12 _left">DENIS ROVIRA</h1>';
        video_gallery_html += '   <h3 class="title_more col span_12 _right">See all projects</h3></div><!-- title_div -->';
        video_gallery_html += '   <a href="#"> <img src="'+ img_src +'" alt="image_item" /></a>';
        video_gallery_html += ' <p>Nescafe Gold</p></article><!-- span_8 -->';


        $(container).prepend(video_gallery_html);
}

function gallery_images(xmldata){

        for (var i = 0; i <= 2; i++) {
            addImage(xmldata[i], '.videos_container')
        }


        for (var i = 3; i <= 5; i++) {
            addImage(xmldata[i], '.videos_container2');          
        };

}

I agree with jcmalpizar's answer in that you will probably find a template engine helpful. You could create the objects directly in the original array and use the objects within the addImage function.

$(data).find("image").each(function() {
     xmldata.push({ img_src: $(this).attr("src"), href: $(this).attr("href"), title1: $(this).attr("title1"), client: $(this).attr("client") }); 
});
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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