5
\$\begingroup\$

I am now working on a Node.js project, and on my ejs view page, I'm retrieving this array of objects, like:

[{ name: 'Andy',
   age: 20,
   username: 'andygoodluck'},
  {name: 'Terry',
   age: 21,
   username: 'terrygoodnight'}]

My objective is to make use of this data to dynamically create and inject content using JavaScript. My existing approach is:

  1. Put this data into a variable, say usersObjArr.
  2. Set number of user blocks to be shown per row with a variable, say numUsersPerRow=3.
  3. Calculate the number of rows I will need, like numRows=getNumRows(usersObjArr.length);
  4. If there is only one row of content needed, simply go through the array of objects and push them into the row:

    for(var j=0;j<usersObjArr;j++){
      insertUserBlock();//pushes user objects one by one into the row
    }
    
  5. Else if there is more than one row, check that I am not working with the last row. As long as it is not the last row, insert the predetermined number of user blocks using the array of objects. Like:

    for(var i=0;i<numRows;i++){
      if(i!=(numRows-1)){
      //as long as it's not the last row, insert 3 users
      for(var k=0;k<numUsersPerRow;k++){
          insertUserBlock(i); //insert 3 blocks into the row specified by the argument
      }
    
  6. On the other hand, if it is the last row, then I will insert the number of users equivalent to the remainder:

    else{
        for(var k=0;k<usersObjArr.length%numGamesPerRow;k++){
            insertUserBlock(i);
        }
    }
    

I can't help but feel that this is not the most efficient approach. I am a web designer turned frontend developer and have a weak programming background, so I can really do with some advice on how to improving my coding above.

==========UPDATED==========

Here's the full code:

function getNumRows(numOfUsers){
    return Math.ceil(numOfUsers/3);
}

function insertUserBlocks(usersObjArr){
    var numUsersAdded=0,
    numUsersPerRow=3,
    numRows=getNumRows(usersObjArr.length);
    if(usersObjArr.length<numUsersPerRow){
    //if only one row worth of users, then insert just one row and push userblocks based on number of users
    insertUserRow();
    for(var j=0;j<usersObjArr.length;j++){
        insertUserBlock();
    }
    }
    else{
    //if more than 3 users, we need to insert more than one row, and insert users into first row, then second, and so on
    var numUsers=usersObjArr.length;
    for(var i=0;i<numRows;i++){
        insertUserRow(i);
        if(i!=(numRows-1)){
        //as long as it's not the last row, insert 3 users
        for(var k=0;k<numUsersPerRow;k++){
            insertUserBlock(i);
            insertUserBlockContent(i,k,usersObjArr[numUsers-1]);//insert userContent based on last index of usersObjArr; minus one because of zero-based index
            numUsersAdded++;
            numUsers--;
        }
        }
        else{
        //otherwise, insert number of users equivalent to the remainder
        for(var l=0;l<usersObjArr.length%numUsersPerRow;l++){
            insertUserBlock(i);
            insertUserBlockContent(i,l,usersObjArr[numUsers-1]);
            numUsersAdded++;
            numUsers--;
        }
        }
    }
    }
}

function insertUserRow(rowNumber){
    if(arguments.length==0)
    $('#main').append('<div class="row-fluid" />');
    else
    $('#main').append('<div class="row-fluid" id="row'+rowNumber+'" />');
}

function insertUserBlock(rowNum){
    if(arguments.length==0){
    $('#main .row-fluid').append('<div class="span4 user-block" />');
    }
    else{
    $('#row'+rowNum).append('<div class="span4 user-block" />');
    }
}

function insertUserBlockContent(rowNum,blockNum,userObj){
    var userName=userObj['name'],
    userPic=userObj['pic'],
    userNickname=userObj['nickname'],
    userUrl=userObj['url'];
    $('#row'+rowNum+' .span4').eq(blockNum).append('<img src="'+userPic+'" />').append('<div />');
    $('#row'+rowNum+' .span4 div').eq(blockNum).append('<h2>'+userName+'</h2>').append('<p>'+userNickname+'</p>').append('<a target="_blank" class="center btn btn-danger" href="'+ userUrl +'">Website</a>');
}

Basically what I'm trying to achieve is to extract the data and use it to insert rows of user content into the page. Maybe it's because of the rigid HTML structure in place? (I'm using Bootstrap.) I feel like my approach isn't quite efficient.

\$\endgroup\$
1
  • \$\begingroup\$ Hello and welcome to Code Review! I'm not sure I understand what you're trying to do. Can you describe a bit more your goal here? What does insertUserBlock do? Can we see all the code you're describing here? Thank you. \$\endgroup\$ Dec 9, 2013 at 13:16

2 Answers 2

2
\$\begingroup\$
function createUserList(users) {
    var $table = $('<div class="user-table">');
    for (var i = 0; i < users.length; i++) {
        if (i % 3 === 0) {
            var $row = $('<div class="row">').appendTo($table);
        }
        $row.append(createUserBlock(users[i]));
    }
    return $table;
}

function createUserBlock(user) {
    return $('<div>', {'class': 'user-block', 'text': user.name});
}

$('#main').append(createUserList(users));

Instead of querying and updating elements in the document with $('#row'+rowNum+' .span4 div') try to build a detached fragment, and insert it as one whole. The difference in speed is very noticeable on large data sets.

Try to access elements through variables. You can create a row and then store it in a variable $row, and then access it as $row.append(...). This is both easier to read and faster than $('#row'+rowNum+' .span4').append(...).

'<h2>'+userNickname+'</h2>' - there are several reasons why you shouldn't concatenate unescaped plain text with HTML, but the main reason is cross-site scripting. Imagine if some user's nickname is <script>location = 'http://pwned.com/'</script>. You probably don't want to allow someone to redirect your visitors to another site, or create fake login forms, or steal cookies. Here's how you can avoid that: $('<h2>').text(userName)

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your advice! Your code is so much more compact and cleaner compared to mine. I especially love the line if (i % 3 === 0) that checks if we are coming to a new row. I've learned a lot through your code. Cheers. :) \$\endgroup\$
    – anthonytwp
    Dec 23, 2013 at 9:13
1
\$\begingroup\$

The most simple and probably what will save you some time in the future is to wrap every JSON object into a <div> and add all of your wrapper divs into another div (or whatever element that might be best for your purpose). Then just style it using CSS.

Here is an jsfiddle example (this could be improved ten-folds, as it's just a proof of concept)

So as you can see, a very simple holder div (rename as you see fit), the given data, an (unoptimized) loop to iterate every item in your array, and a very small CSS.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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