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.

The following working code basically swaps the content of two divs. The divs are created dynamically, then the user checks which records they want to swap, and then clicks on a swap button that triggers a function to swap an inner div element.

The part that swaps the div's seems really messy to me. I'm only a beginner/intermediate with JavaScript, so I would really appreciate some help with this.

//dynamically create the div elements
$('#SwapBedsFields').html(options);
   for (var i = 1; i <= NumberOfBeds; i++) {
        var RowIndex = $.inArray(i.toString(), bedNumberColumn);
        var ptName = 'Empty';
        var UMRN = "";
        var UMRNstr = '';
        var pID = '';

       if (RowIndex != -1) {
           ptName = patientNameColumn[RowIndex];
           UMRN = patientUMRNColumn[RowIndex];
           pID = pIDColumn[RowIndex];
           UMRNstr = "(" + UMRN + ")";
       };
       options += '<div style="white-space:nowrap; height:25px; width:100%;" id="bed-topdiv-' + i + '"><input type="checkbox" style="vertical-align:middle;" name="bed-' + i + '"';
       options += 'id="bed-' + i + '" /><div style="display:inline; vertical-align:middle;">&nbsp;Bed ' + i + '  -  </div><div id="bed-div-' + i + '" class="inner-bed-div-class" style="display:inline; vertical-align:middle;" bedNum=' + i + ' uMRN=' + UMRN +' >' + ptName + '&nbsp;' + UMRNstr + '</div></div>';      
};


//***THIS IS THE BIT I FIND PARTICULARLY MESSY!***
//then in another function - when the user clicks a button to swap the div elements...
var selected = new Array();
var patientname1;
var patientname2;
var b1html;
var b2html;
var pt1UMRN;
var pt2UMRN;

$('#SwapBedsFields input:checked').each(function () {
       selected.push($(this).attr('name'));
});

//get the html of the two bed divs
patientname1 = $('#bed-div-' + selected[0].substring(4)).html();
patientname2 = $('#bed-div-' + selected[1].substring(4)).html();
pt1UMRN = $('#bed-div-' + selected[0].substring(4)).attr('umrn');
pt2UMRN = $('#bed-div-' + selected[1].substring(4)).attr('umrn');


//swap the elements around
$('#bed-div-' + selected[0].substring(4)).html(patientname2);
$('#bed-div-' + selected[1].substring(4)).html(patientname1);

//update the umrn attribute
$('#bed-div-' + selected[0].substring(4)).attr({ umrn: pt2UMRN});
$('#bed-div-' + selected[1].substring(4)).attr({ umrn: pt1UMRN });

//message the user
$('#bed-topdiv-' + selected[0].substring(4)).effect("highlight", {}, 2000);
$('#bed-topdiv-' + selected[1].substring(4)).effect("highlight", {}, 2000);
share|improve this question

1 Answer 1

You could improve it a little like this, at least you are not selecting the same elements over and over.

I do think it could improve more, if you swapped the actual div's rather then the html content and umrn value.

var $bed0 = $('#bed-div-' + selected[0].substring(4));
var $bed1 = $('#bed-div-' + selected[1].substring(4));

//get the html of the two bed divs
patientname1 = $bed0.html();
patientname2 = $bed1.html();
pt1UMRN = $bed0.attr('umrn');
pt2UMRN = $bed1.attr('umrn');


//swap the elements around
$bed0.html(patientname2);
$bed1.html(patientname1);

//update the umrn attribute
$bed0.attr({ umrn: pt2UMRN});
$bed1.attr({ umrn: pt1UMRN });

//message the user
$bed0.effect("highlight", {}, 2000);
$bed1.effect("highlight", {}, 2000);
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.