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 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?
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?
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?
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?