JS code refactoring

Question

I do a dynamic search, i.e. I add and remove items (in the table) on the fly. Is it possible to somehow reduce the code? data_parse is an associative array received from the server.

 let data_parse = JSON.parse(data); console.log(JSON.parse(data)); $('#listDataEmployee').remove(); let el_tbody = document.createElement('tbody'); for (let i = 0; i < data_parse.length; i++) { let tagTrDataEmployee = document.createElement('tr'); let tagTd_id = document.createElement('td'); tagTd_id.innerHTML = data_parse[i]['id']; tagTrDataEmployee.append(tagTd_id); let tagTd_email = document.createElement('td'); tagTd_email.innerHTML = data_parse[i]['email']; tagTrDataEmployee.append(tagTd_email); let tagTd_fio = document.createElement('td'); tagTd_fio.innerHTML = data_parse[i]['FIO']; tagTrDataEmployee.append(tagTd_fio); let tagTd_phone = document.createElement('td'); tagTd_phone.innerHTML = data_parse[i]['phone']; tagTrDataEmployee.append(tagTd_phone); let tagTd_role = document.createElement('td'); tagTd_role.innerHTML = data_parse[i]['role']; tagTrDataEmployee.append(tagTd_fio); el_tbody.append(tagTrDataEmployee); } $('.listNewDataEmployee').append(el_tbody);

Yes, it contains less explanation, but it is suitable for use without an understanding of what is happening there (consider it is not necessary to remember about the potential xss).

Pavel mayorov 48.9k five 50 110 · Accepted Answer · 2016-12-28T08:51:08

I guess you really want something like this:

 var data = `[ {"id": 1, "email": "first@mail.ru", "FIO":"First Firstov", "phone": "+7 (111) 111 11 11", "role": "admin"}, {"id": 2, "email": "second@mail.ru", "FIO":"Second Secondov", "phone":"+7 (222) 222 22 22", "role": "user"}, {"id": 3, "email": "xss@mail.ru", "FIO":"<img src='http://void' onerror='alert(1)'>", "phone":"+7 (333) 333 33 33", "role": "test"} ]`; data = JSON.parse(data); $('#listDataEmployee').replaceWith( data.map(d => $("<tr>").append([ $("<td>").text(d.id), $("<td>").text(d.email), $("<td>").text(d.FIO), $("<td>").text(d.phone), $("<td>").text(d.role), ])));

 table, tr, td, th { border-collapse: collapse; border: 1px solid; }

 <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script> <table> <thead> <tr> <th>ID <th>Email <th>FIO <th>Phone <th>Role </tr> <tbody id="listDataEmployee"> </table>

And why should we expand the result of a map into an array through three points - if the map already returns an array?
The question is not written about jquery, it is not known what it will deploy this code.
@Ivan, map is another function that bypasses an array, often more useful.
map - converts each element and returns the converted elements as a new array.
@Ivan, they can be replaced with each other with minimal additions, the difference between them is mostly semantic (when the resulting array is needed is a map, when not needed is forEach).
And .map is shorter and more fashionable.) You can ask a separate question, I think you will quickly explain it with examples, in the comments it is inconvenient

Answer 2 · 2016-12-27T22:46:08

The first thing that comes to mind is to combine the same type of conditions in some way:

 // Перед циклом, чтобы не создавать этот объект в каждой итерации let columnsList = [ 'id', 'email', 'FIO', 'phone', 'role' ]; // Немножко кода пропущено // Внутри тела цикла columnsList.forEach(columnName => { var tdEl = document.createElement('td'); tdEl.innerHTML = data_parse[i][columnName]; tagTrDataEmployee.append(tdEl); });

I am not capable of more complex optimizations at 2 am)
I can also say if the table is really very large, then it is faster to glue the string, and then make DOM elements from it, but here it does not matter.

You can also not hardcore a sheet of columns and take straight from the object all that came:

 let columnsList = Object.keys(data_parse[i]);

(As Qwertiy rightly noted, Object.keys can return keys in the wrong order that you have in the header, and this must be borne in mind)

UPD: looked more attentively in the morning. You use jQuery, which greatly simplifies the optimization described above.
You can write something in the spirit of $('<div>test</div>') , that is, feed a jQuery piece of html written as a string, and he will parse it himself. As a result, the piece creating the string can be written down quite simply:

 let rowString = `<tr>`; columnsList.forEach(columnName => { rowString += `<td>${data_parse[i][columnName]}</td>`; }); rowString +=`</tr>`; let rowEl = $(rowString);

(I was too lazy to check the performance, but I think the idea is clear).
Attention: as Qwertiy correctly noted that if the data that you show is entered by the user and is not validated, there is a chance to skip xss.
But if it shows some pre-valid data, then it can be so.

Why does this make sense? The fact is that operations with js strings themselves are very fast. DOM change operations are much slower. Calling some browser APIs outside of the js engine is generally slower.

Yes, note that I use ES6 constructs in the code (=> - pointer functions, `` - patterned strings), but since you already have a let, I assumed that there were no problems with that.

And with Object.keys , the order may not match the order of the columns.
You give some kind of bad advice ... Although stop, there and in the original html, it is strange.
I did not think about the order Object.keys, I agree (I wrote at 2 am =)).
I was too lazy to watch about createElement, that's why I wrote "probably" =)
@Qwertiy, well, I agree like that) Well, yes, this is a potential xss, but in some cases it can of course be so)

JS code refactoring

2 answers 2

More articles: