7
\$\begingroup\$

My JavaScript is fairly new and want to learn new ways to create clean, fast code. Can you have a look at this code and guide me on how to optimise this code to improve my skills? I have provided the HTML even though I am not able to edit the actual html as its generated but lets pretend I could..

What I am doing is pulling data from a Product JSON file and manipulating the data to be shown in the website.

function Details(el){
    var sku = $(el).attr("data");
    $.getJSON("/micro/"+sku, function(json) {
        if (json.price!=null) {
          $(el+' .p_img img').attr({src:json.images[2].url, alt:json.images[2].altText});
          $(el+' .p_img_lrg').attr("src",json.images[1].url);
          $(el+' .p_name').text(json.name);
          $(el+' .p_url').attr("href",json.url);
          $(el+' .p_p').text("£"+Number(json.p.value).toFixed(2));
          $(el+' .p_pu').text(json.pu);
          $(el+' .p_b_name').text(json.categories[0].name);
          $(el+' .p_b_url').attr("href",json.categories[0].url);
        if (json.oldPrice != null) {
          var pWas = ' .p_was',
              pSave = ' .p_save';
          $(el+pWas).text("was £"+json.old.value);
          $(el+pSave).text("save £"+(json.old.value-json.value));
          $(el+pSave).hide();
          $(el+pWas).css('margin-left',0).css('padding-left',0).addClass('clr').removeClass('left').css('float','none');
        }else{
          $(el+pSave,el+pWas).hide(); 
        }
    }

    if (json.stockLevel==0) { 
      $(el+' form.add').attr({'action':'/notification','method':'post'}).html('<input type=\"hidden\" name=\"productCodePost\" value=\"'+sku+'\"><input type="submit" value="Out">');
    } else { 
      $(el+' form.add').attr({'action':'/add','method':'post'}).html('<input type=\"hidden\" name=\"productCodePost\" value=\"'+sku+'\"><input type=\"hidden\" name=\"maxOrderQuantity\" value=\"\"><input type="submit" value="Add">');
    }
});
}

HTML:

<li data-sku="100000" class="item1">
    <div class="bullet">
        <sup class="hash">#</sup>
        <span>test</span>
    </div>
    <div class="image">
        <a class="p_url" title="" href="#"><img class="p_img_lrg" border="0" alt="" src="" width="210" height="210" /></a>
    </div>
    <div class="prodinfo">
        <h2 class="nomargin"><a class="p_name p_url" href="#"></a></h2>
        <div class="more-less">
            <div class="more-block"><p></p></div>
        </div>
            <div class="price">
                <div class="now pink bold large-text">
                    <span class="pricename">now</span> 
                    <span class="p_price">&pound;</span>
                </div>
            <div class="p_price_unit margin10bottom"></div>
        <form class="add_to_cart_form">&nbsp;</form>
     </div>
     <div class="morefrom">View All <a class="p_brand_name p_brand_url" title="" href="#"></a> Products</div>
  </div>
</li>
\$\endgroup\$
2
  • \$\begingroup\$ Could you please describe what this code does, and edit the title accordingly? \$\endgroup\$
    – Jamal
    Mar 19, 2014 at 14:12
  • \$\begingroup\$ And why can't you provide the HTML? It would help a lot to judge if the the selectors are optimal. \$\endgroup\$
    – RoToRa
    Mar 19, 2014 at 16:07

2 Answers 2

5
\$\begingroup\$

Functions in JavaScript, by convention, begin with a lowercase letter, unless they're constuctors. So Details() should be details()

You've got some missing indentation, which makes the code harder to read. In general your code could probably benefit from a bit more whitespace to help legibility.

You also have some funky-looking strings, like: '<input type=\"hidden\" name=\"productCodePost\" value=\"' You use single quotes around the string, but still escape the double-quotes inside the string. This is unnecessary: A single-quoted string can contain double-quotes just fine, and a double-quoted string can contain single-quotes (like so 'double-quotes (") work fine here' or "single-quotes (') work fine here"). You only need to escape if your string contains the same type of quote that you're using to delimit the string.
In fact, you're already doing that in your code. The rest of the line I used as example looks like this '\"><input type="submit" value="Out">' and works just fine with out any backslashes.

You can also use jQuery to build those elements for you, i.e.

form.append($("<input>").attr({
  type:  hidden,
  name:  productCodePost,
  value: sku
});

which gives you a more structured and readable source code, instead of giant strings.

There's also a lot of repetition - or semi-repetition in this case: The el+"...". I'd advice something like what Matt suggested which is to store the result of $(el) once, and from there using .find() to locate the individual elements you want to update.

For the actual element updating there isn't that much that can be improved, since there's no discernible commonalities between, say, element IDs and JSON keys (in fact, the JSON seems very messy!)

There are some minor things you could do like define a renderAmount function to make sure you're always printing prices the same way:

function renderAmount(amount) {
  return "£" + Number(amount).toFixed(2);
}

Right now, you only use the .toFixed() in one place, although you have 3 different places where you need to write amounts.

Similarly, you could add a function to update images, since you do it at least twice. Not a great savings, but it helps break up the code.

function updateImage(element, info) {
  element.attr({
    src: json.url,
    alt: json.altText
  });
}

// ...

updateImage(container.find(".p_img img"), json.images[2]);

Lastly, what's going on the element that shows the savings? If there's no old price, it's just hidden, which makes sense. But if there is an old price, you update the .p_save element... and then hide it anyway?

\$\endgroup\$
2
  • \$\begingroup\$ Thanks both for you help. Another question, after apply your changes I have alot of $el.find('selector'), which to me seems like there should be a way to loop this? The code still looks bulky.. Any suggestions? \$\endgroup\$
    – Niraj paul
    Mar 20, 2014 at 14:07
  • 1
    \$\begingroup\$ @Nirajpaul Not really. The trouble is that the JSON keys don't match up with the element IDs/attributes. You have to manually match the JSON content to the HTML elements. It can't, as far as I can tell, be automated. \$\endgroup\$
    – Flambino
    Mar 20, 2014 at 17:04
4
\$\begingroup\$

Caching the jquery set for $(el) and using it to make subqueries should be a little faster, and I usually find it to be more readable. For example:

var $el = $(el);
//snip
$el.find('.p_img img').attr({src : json.images[2].url, alt : json.images[2].altText});

I always use the identity operator unless I specifically want type conversion to happen. You might get some unexpected results using the equality operator to compare against values like 0 or null (MDN equality operator docs) :

if (json.price !== null) { 

if (json.stockLevel === 0) { 
\$\endgroup\$

Your Answer

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

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