3
\$\begingroup\$

I'm updating and old project from jQuery 1.4 to jQuery 1.7.

Can this be simplified, thus reducing the amount of code, and perhaps in such reduction, have it improved?

Having the following code:

var cartItemDel   = '<td class="del"><img src="components/themes/default/img/icons/cross-small.png" width="16" height="16" alt="X"><input type="hidden" class="lid" name="lineID'+ id +'" value="'+ id +'"><span class="hidden token">'+token+'</span></td>',
    cartItemName  = '<td><input class="CartItemId" type="hidden" name="id_'+ id +'" value="'+ id +'"><span class="CartItemName">'+ name +'</span></td>',
    cartItemPrice = '<td><input class="CartItemRef" type="hidden" name="ref_'+ ref +'" value="'+ ref +'"><span class="class="CartItemPrice"><span class="lineSum">'+ price +'</span>&euro;</span></td>';

$(".cart-table tbody")
    .append('<tr>'+cartItemDel+cartItemName+cartItemPrice+'</tr>');

I've updated it to:

/*
 * ADD NEW LINE - First TD
 */
var $newLineIcon = $("<img/>", {
        src    : "components/themes/default/img/icons/cross-small.png",
        width  : "16",
        height : "16",
        alt    : "X"
    });

var $newLineLID = $("<input/>", {
        type  : "hidden",
        class : "lid",
        name  : "lineID"+ lid,
        value : lid
    });

var $newLineToken = $("<span/>", {
        class : "hidden token",
        text  : token
    });

var $newLine_firstTD = $("<td/>", { class : "del"})
    .append($newLineIcon)
    .append($newLineLID)
    .append($newLineToken);

/*
 * ADD NEW LINE - Second TD
 */
var $newLinePID = $("<input/>", {
        type  : "hidden",
        class : "CartItemId",
        name  : "id_"+ id,
        value : id
    });

var $newLinePNAME = $("<span/>", {
        class : "CartItemName",
        text  : name
    });

var $newLine_secondTD = $("<td/>")
    .append($newLinePID)
    .append($newLinePNAME);

/*
 * ADD NEW LINE - Third TD
 */
var $newLinePREF = $("<input/>", {
        type  : "hidden",
        class : "CartItemRef",
        name  : "ref_"+ ref,
        value : ref
    });

var $newLinePPRICE = $("<span/>", {
        class : "CartItemPrice",
        html  : "&euro;"
    })
    .prepend($("<span/>", {class : "lineSum", text : price}));

var $newLine_thirdTD = $("<td/>")
    .append($newLinePREF)
    .append($newLinePPRICE);

$("<tr/>")
    .append($newLine_firstTD)
    .append($newLine_secondTD)
    .append($newLine_thirdTD)
    .appendTo($(".cart-table tbody"));
\$\endgroup\$
3
\$\begingroup\$

+1 to this code looking fine as is (with caveats and one issue).

Issue: You should always quote class as it is a reserved word for future use in javascript.

Don't take this as me advocating you use it or saying it is ready for production1, but I'm using JsRender in production to ease much of the pain you are seeing/imagining here.

I think the central issue here is that this type of Javascript invokes a hard dependency on the structure of the page and so we look at it with a bit of queasiness that it rightly deserves. If your visual designer comes along and decides that the shopping cart needs to be in a dropdown list where you can drag (or swipe) items out of or into now you have to go and make a whole bunch of changes all over the place.

Unfortunately I don't think it is reasonable in a modern website to expect both that the html has no dependencies on the Javascript ("page works without javascript") and that the Javascript has no dependencies on the design of the html("javascript works equally well on a totally different design that happens to have a couple of the same named elements").

I personally am comfortable ignoring the former to get much closer to an ideal on the latter. That said, my working environment is such that I have very little control over the redesigns that happen all too often in the products I work on.

At a minimum I would strive to make sure this code stayed together inside its own function at or near the top of your javascript (or in its own global/namespaced function in the html file itself). This code has very little to do with your logic and a lot to do with your site design. Therefore it doesn't belong sitting alongside your page logic.

On minification (for this code in particular) I am less concerned. A solid minifier is going to combine your var statements, rename the variables and possibly inline them and then remove whitespace. If you were concerned about this, you could inline everything yourself (and you would lose a little on the readibility), but the gzip of this even uncompressed is only about 480 bytes (closure compiler adds some stuff before it estimates) so at most you are going to gain maybe 100 bytes (closure advanced gets 140 but the result will not work; I can manually do it and get 145 off; none of these numbers will mean anything in terms of page performance for your site).

FWIW, your original code was only 332 bytes zipped (smaller than my manual attempt) and compressed (manually) down to 297.


1. The tests are poorly covering the functionality (willing to bet there are significant issues in various places) and it is sorely missing even half decent documentation.

\$\endgroup\$
  • \$\begingroup\$ I did find weird the code highlight for the "class" word, now I know why! Tks. \$\endgroup\$ – Zuul Jul 10 '12 at 10:46
  • \$\begingroup\$ You are right as to the code being very dependent on the HTML Markup, but that's normal to me, since the solution being implemented is very specific to this client. \$\endgroup\$ – Zuul Jul 10 '12 at 10:47
  • \$\begingroup\$ This piece of code is inside a function and it is called more than once with a different CSS, depending on the website area. Also, it's already at the very beginning of the JS file that contains code related to the cart presentation without page refresh. \$\endgroup\$ – Zuul Jul 10 '12 at 10:47
  • \$\begingroup\$ Related to the code minify, although this piece of code as is doesn't require it, I'm doing it to the entire file since there much more to it that this. \$\endgroup\$ – Zuul Jul 10 '12 at 10:48
  • \$\begingroup\$ Thank you for taking the time to analyze several potential issues from this code and its surroundings. I've going it with your advice on the reserved word! \$\endgroup\$ – Zuul Jul 10 '12 at 10:49
4
\$\begingroup\$

Your code looks OK to me, very clear and organised.

It's more important to minify your code to decrease load time. There's a whole bunch of minification tools on the Internet.

\$\endgroup\$
  • 2
    \$\begingroup\$ Although your response was the first one, and indicated in a simple manner the ideal procedure regarding the code as a whole (minify), You didn't provide a more extensive analysis in relation to this piece of code, being that what I was looking for. I've +1 your answer nonetheless because you deserve it. Thank you for you time. ps: thank you for the complement on my code presentation :) \$\endgroup\$ – Zuul Jul 10 '12 at 10:46
  • \$\begingroup\$ @Zuul: I think it should've been a comment instead, but he's already received the upvotes. :-) \$\endgroup\$ – Jamal Jun 21 '13 at 2:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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