Rendering three types of events as HTML elements

Question

I have to clean up some Javascript and I am trying to figure out a more elegant way to write this code below. It's basically looping through 3 dictionaries with the same loop structure and appending some custom HTML onto a specific div. I wanted to see if there is a way to avoid this repetitive code:

if (dtInt in calendarEventDict) {
 var arrEvents = calendarEventDict[dtInt];
 for (var a = 0; a < arrEvents.length; a++) {
 var calendarEvent = arrEvents[a];
 $("#dateCupcake" + i).append("<div dateId='" + calendarEvent.Date + "' class='event " + "cal" + calendarEvent.CalendarId + " " + calendarEvent.CalendarClassName + "' calendarId='" + calendarEvent.CalendarId + "' eventDateId='" + calendarEvent.Id + "' id='" + calendarEvent.EventId + "'>" + calendarEvent.DisplayName + "</div>");
 }
}
if (dtInt in vacationEvents) {
 var arrEventsV = vacationEvents[dtInt];
 for (var v = 0; v < arrEventsV.length; v++) {
 var vacationEvent = arrEventsV[v];
 $("#dateCupcake" + i).append("<div dateId='" + vacationEvent.Date + "' class='event " + "cal1 blueAllDay' calendarId='1' eventDateId='" + vacationEvent.Id + "' id='" + vacationEvent.EventId + "'><img src='/Content/Images/Icons/pawn_glass_" + vacationEvent.ApprovalIcon + ".png' />" + vacationEvent.PersonName + "</div>");
 }
}
if (dtInt in travelEvents) {
 var arrEventsV1 = travelEvents[dtInt];
 for (var v1 = 0; v1 < arrEventsV1.length; v1++) {
 var travelEvent = arrEventsV1[v1];
 $("#dateCupcake" + i).append("<div calendarId='2' dateId='" + travelEvent.Date.ToString("MMM dd, yyyy") + "' class='event " + "cal2" + " blueAllDay' calendarId='2' eventDateId='" + travelEvent.Id + "' eventId='" + travelEvent.TravelRequest.Id + "'><img src='/Content/Images/Icons/user1_earth16.png' />" + travelEvent.TravelRequest.Person.LastName + ", " + travelEvent.TravelRequest.Person.FirstName.Substring(0, 1) + " visiting " + travelEvent.TravelRequest.TechnicalCentre.Name + "</div> ");
 }
}

• Raynos: HTML String concatenation is evil.
• leora: @Raynos - what would you suggest as an alternative
• Raynos: Construct objects using the DOM
• James Khoury: @Raynos I agree the DOM way is better but assuming there are only a few items here its not a big deal. Simpler to let jQuery handle it.
• Raynos: @JamesKhoury we're not talking about performance differences. Were talking about the DOM aint no bloody string. Stop writing HTML in my javascript.

Answer 1

The simplest refactor would be to simply pull out the common functionality, and pass in the array for each case, as well as a function used to create your div content from the current item in the loop. Assuming dtInt is global:

function checkDtInt(arr, funcToCreateDiv) { 
 if (dtInt in arr) {
 var arrEventsV1 = arr[dtInt];
 for (var v1 = 0; v1 < arrEventsV1.length; v1++) {
 var travelEvent = arrEventsV1[v1];
 $("#dateCupcake" + i).append(funcToCreateDiv(travelEvent));
 }
 }
 }
checkDtInt(calendarEventDict, function(item) { 
 return "<div dateId='" + item.Date + "' class='event " + "cal" + item.CalendarId + " " + item.CalendarClassName + "' calendarId='" + item.CalendarId + "' eventDateId='" + item.Id + "' id='" + item.EventId + "'>" + item.DisplayName + "</div>"
});
checkDtInt(vacationEvents, function(item) { 
 return "<div dateId='" + item.Date + "' class='event " + "cal1 blueAllDay' calendarId='1' eventDateId='" + item.Id + "' id='" + item.EventId + "'><img src='/Content/Images/Icons/pawn_glass_" + item.ApprovalIcon + ".png' />" + item.PersonName + "</div>"
});
checkDtInt(travelEvents, function(item) { 
 return "<div calendarId='2' dateId='" + item.Date.ToString("MMM dd, yyyy") + "' class='event " + "cal2" + " blueAllDay' calendarId='2' eventDateId='" + item.Id + "' eventId='" + item.TravelRequest.Id + "'><img src='/Content/Images/Icons/user1_earth16.png' />" + item.TravelRequest.Person.LastName + ", " + item.TravelRequest.Person.FirstName.Substring(0, 1) + " visiting " + item.TravelRequest.TechnicalCentre.Name + "</div> "
});

Answer 2

(削除) This is Less code but I wouldn't say simpler: (削除ここまで) EDIT: Its not even less code any more. Less repeated code though.

var groups =[
 {
 events: calenderEventDict,
 calendaridfn: function(){ return this.calendarId;},
 contentsfn: function(){ return this.DisplayName; },
 datefn: function(){ return this.Date;}
 },
 {
 events: vacationEvents,
 calendaridfn: function(){ return "1";},
 contentsfn: function(){
 var contents = jQuery("<img/>")
 .prop("src", "/Content/Images/Icons/pawn_glass_" + this.ApprovalIcon + ".png")
 .after(this.PersonName);
 return contents;
 },
 datefn: function(){ return this.Date;}
 },
 {
 events: travelEvents,
 calendaridfn: function(){ return "2";},
 contentsfn: function(){ 
 var contents= jQuery("<img />")
 .prop("src", '/Content/Images/Icons/user1_earth16.png')
 .after(
 this.TravelRequest.Person.LastName + ", " +
 this.TravelRequest.Person.FirstName.Substring(0, 1) + 
 " visiting " +
 this.TravelRequest.TechnicalCentre.Name
 );
 return contents;
 },
 datefn: function(){ return this.Date.ToString("MMM dd, yyyy"); }
 }
];
jQuery.each(groups, function(){
 if (dtInt in this.events) 
 {
 var group = this;
 jQuery.each(events, function(){
 var calendarId = group.calendaridfn.call(this);
 $("#dateCupcake" + i).append(
 jQuery("<div/>").prop({
 dateId: group.datefn.call(this),
 class: ["event",
 "cal" + calendarId,
 (this.CalendarClassName || "blueAllDay")].join(" "),
 calendarId: calendarId,
 eventDateId: this.Id, 
 id: this.EventId 
 })
 .append(group.contentsfn.call(this))
 );
 }
 }
});

EDIT:- So after a discussion with Raynos I've come to the understanding that its better to not have a huge html string over the object creation method. I've edited my answer to reflect this idea.

Answer 3

There's many ways of doing this. But no need to over complicate things, your code isn't bad at all. Could use some optimisation though, that's all.

1. So let's start by optimizing your code. You can get faster results in slower machines with a small simple refactoring. That's also clean. I presume that you don't need tp declare new variables for each of the loops. Meaning you can use the same ones.

2. Not always a standard, but good practice. To use 'single Quotes' and keep the "double quotes for HTML" when possible

   // have all you need here, only once. These variables can be used for more than one loop.
   var events,
    eventsLen,
    travelRequest,
    ev,
    i;
   if (dtInt in calendarEventDict) {
    events = calendarEventDict[dtInt];
    i = arrEvents.length;
    // loop until we reach 0. Less calls, less checks ;)
    while(--i) {
    ev = arrEvents[a];
    $("#dateCupcake" + i).append("<div dateId='" + ev.Date + "' class='event " + "cal" + ev.CalendarId + " " + ev.CalendarClassName + "' calendarId='" + ev.CalendarId + "' eventDateId='" + ev.Id + "' id='" + ev.EventId + "'>" + ev.DisplayName + "</div>");
    }
   }
   if (dtInt in vacationEvents) {
    events = vacationEvents[dtInt];
    i = arrEventsV.length;
    while(--i) {
    ev = arrEventsV[v];
    $("#dateCupcake" + i).append("<div dateId='" + ev.Date + "' class='event " + "cal1 blueAllDay' calendarId='1' eventDateId='" + ev.Id + "' id='" + ev.EventId + "'><img src='/Content/Images/Icons/pawn_glass_" + ev.ApprovalIcon + ".png' />" + ev.PersonName + "</div>");
    }
   }
   if (dtInt in travelEvents) {
    events = travelEvents[dtInt];
    i = arrEventsV1.length;
    while(--i) {
    ev = arrEventsV1[v1];
    travelRequest = ev.TravelRequest;
    $("#dateCupcake" + i).append("<div calendarId='2' dateId='" + ev.Date.ToString("MMM dd, yyyy") + "' class='event " + "cal2" + " blueAllDay' calendarId='2' eventDateId='" + ev.Id + "' eventId='" + travelRequest.Id + "'><img src='/Content/Images/Icons/user1_earth16.png' />" + travelRequest.Person.LastName + ", " + travelRequest.Person.FirstName.Substring(0, 1) + " visiting " + travelRequest.TechnicalCentre.Name + "</div> ");
    }
   }