Iterating through a linked-list in a cleaner manner

Question

I have a collection of PlaylistItem objects. They are linked together such that each item knows the ID of its next/previous item. I am iterating over this collection of objects, starting at a known position and working with each object once. I'm not a big fan of how I do this, but I am not seeing an obvious way of rewriting it.

var firstItemId = activePlaylist.get('firstItemId');
var currentItem = activePlaylist.get('items').get(firstItemId);

//  Build up the ul of li's representing each playlistItem.
do {

    var listItem = $('<li/>', {
        'data-itemid': currentItem.get('id'),
        contextmenu: function (e) {

            var clickedItemId = $(this).data('itemid');
            var clickedItem = activePlaylist.get('items').get(clickedItemId);

            contextMenu.initialize(clickedItem);

            //  +1 offset because if contextmenu appears directly under mouse, hover css will be removed from element.
            contextMenu.show(e.pageY, e.pageX + 1);
            //  Prevent default context menu display.
            return false;
        }
    }).appendTo(playlistItemList);

    $('<img>', {
        'class': 'playlistItemVideoImage',
        src: 'http://img.youtube.com/vi/' + currentItem.get('video').get('id') + '/default.jpg',
    }).appendTo(listItem);

    $('<a/>', {
        text: currentItem.get('title')
    }).appendTo(listItem);

    currentItem = activePlaylist.get('items').get(currentItem.get('nextItemId'));
} while (currentItem && currentItem.get('id') !== firstItemId)

How would you write this?

Note that it is a do-while because I want to ensure I always render that firstItem. If there's only one item in the list, the loop will immediately terminate. Thus the need for the do-while.

Answer 1

Here's a bit of "light" refactoring. I've basically moved the element constructing into its own function to keep the while-block less crowded.

I'm also using some closure magic for the contextmenu, so it doesn't have to re-get the clicked item.

Lastly, I'm going more by the current ID rather than the current object when looping. This was to avoid the "get the id to get the object to get its id again" code that I see a few places in your version.

function buildListItem(item) {
  var listItem = $('<li/>', {
    contextmenu: function (event) {
      contextMenu.initialize(item); // available via closure
      contextMenu.show(e.pageY, e.pageX + 1);
      return false;
    }
  });

  listItem.append($('<img>', {
    'class': 'playlistItemVideoImage',
    'src':   'http://img.youtube.com/vi/' + item.get('video').get('id') + '/default.jpg'
  }));

  listItem.append($('<a/>', {
    text: item.get('title')
  }));

  return listItem;
}

var firstItemId   = activePlaylist.get('firstItemId'),
    allItems      = activePlaylist.get('items'),
    currentItemId = firstItemId,
    currentItem;

do {
  currentItem = allItems.get(currentItemId);             // get the obj
  if(!currentItem) {
    break;                                               // stop if it ain't there
  }
  playlistItemList.append(buildListItem(currentItem));   // build the list item element
  currentItemId = currentItem.get('nextItemId');         // get the next ID
} while(currentItemId && currentItemId !== firstItemId); // I'm assuming that valid IDs are thruth'y

Another thing to consider would be to use rawer HTML because it can be faster.