$(document).on('click', '#updateBtn', function () { // See "Event Handler"
var locations = [], // See "Unused Variable"
rows = [], // See "Improper Scope"
content = {};
$('.rowUpdate').each(function (i) { // See "Exactly One" and "Parameters"
var feed = [];
$(this).find('td').each(function (j, v) { // See "Parameters"
var label = $(this).html();
if (j == 0) feed = feed + label; // See "Type Variance"
if (j != 0) {
var input = $("input", this),
// See "Wait, what?"
name = input.attr("name").substring(0, input.attr("name").length),
value = input.val();
//alert(value); // See "Commented Code" and "Alert"
content[name] = value;
//alert(JSON.stringify(content));
}
if (j == 7) {
// See also "JSON"
feed = feed + '=' + JSON.stringify(content);
}
});
alert(feed);
rows.push(feed); // See "Improper Scope"
});
});
Event Handler -
You've attached the event listener to the document which is fine, but you've used an Id selector to filter it to. There can be only one element with in the document referenced by that Id.1 Unless the #updateBtn element is expected to be destroyed and recreated during the documents lifetime, it would be better to specify that element explicitly.
Unused Variable -
The locations
variable is never referenced after it is instantiated, remove it.
Improper Scope -
The rows
variable is pushed to, but never read from. If it is to be useful to other code it will need to be moved out of the event handler's scope.
The content
variable is never used at this level of the scope, move its declaration to the narrowest scope required.
Exactly One -
According to the HTML snippet you've provided, the .rowUpdate
contains the #updateBtn
which forces me to assume means there will be only 1 .rowUpdate
. Update the markup to indicate this element is special by setting the id
instead of the class
attribute.
Because we expect this .each
function to be invoked once it can be flattened into the parent scope.
Parameters -
JavaScript does not allow functions to be overloaded by parameters.2 the i
and v
parameters are never used and can be removed. Parameters are essentially variable declarations and should be named accordingly. j
would be better named index
.
Type Variance -
feed
is originally instantiated as an empty array, but you're now adding a string to it. I'm not sure what your intent is, but the effect will be to cast the empty array to an empty string and append label
. This is only being done for the 0th element so a simple assignment will do.
Wait, what? - First, nearly every string literal in your code is enclosed in single quotes ('). Suddenly you've begun using double quotes ("). Javascript will allow either, but keep your code consistent.
I'm not sure what the purpose of the .substring
method call when your arguments are the full length of the string. this can be removed.
Commented Code -
Remove code that is unused. Only comment out code for the purposes of debugging or experimenting and remove the comment (and code if necessary) when done.
Alert -
alert
should not be used for debugging purposes. Use the console methods trace
, debug
, log
, warn
, etc..
JSON -
JSON is a format used for serializing data. It should be used to save, restore, and transfer data. Your code should never manipulate JSON directly. Instead use the objects serialized from the JSON string.
After modifying the code based on the review, I end up with something like this:
var rows = [];
$('#updateBtn').on('click', function {
var feed = {},
content = {},
label = $('.rowUpdate td').first().html();
$('tr.rowUpdate td input[type=text]', this).each(function(index, input) {
var $input = $(input),
name = $input.attr('name'),
value = $input.val();
console.debug('value', value);
content[name] = value;
console.debug('content', content);
});
feed[label] = content;
console.debug('feed', feed);
rows.push(feed);
});
Then use the actual objects from the rows array if being consumed in other javascript code or send the rows JSON to wherever it is consumed. If for some reason your consumer cannot be modified to use a proper object and relies on the broken JSON string originally in the method, change feed[label] = content
to feed = label + '=' + JSON.stringify(content)
.
rowUpdate
? Is there a reason to not put the input controls in aform
element? \$\endgroup\$