Review
Hi and welcome to code review.
To help out I have numbered the lines of your code for reference.
I will ignore the console.log
calls as they represent debugging code.
Line 3 Bad function name. You are not uploading a file, you are loading the file and displaying it. That is two separate tasks and should be handled by two separate functions.
It is important that functions do one thing (their role) as describe in the function name. Two functions loadCSV
and displayCSV
and the directing of what to load and where to display is upto the event listing for the CSV file.
Line 5. The variable 'data` is not used. You declare it again in the onload line 11.
You don't need to set a value to null if you have nothing to put in it yet. The default is undefined
.
Lined 6 & 7 The variables file
and reader
should be constants as they dont change.
Line 10 the variable csvData
should be a const
but you only use it once on line 11. Would have been better to add event.target.result
on line 11
Line 11 'data' should be a const
. You are storing the wrong thing in it. If you look at the rest of the code lines 12,13,16,18 you use data.data
. If you assigned the second property data
to data
then you would not have to type data.data
each time.
Thus the line becomes const data = Papa.parse(event.target.result, {header : false}).data;
Line 13 const
for arrayLength
but you don't need it. Tthe 'console.log' on line 12 will have the length and the for loop (line 15) is the only other place you use it. But next point means you don't need it at all.
Line 15 First the i
should be a block scoped variable using let
and data.length
for the length eg for(let i = 0; i < data.length; i++) {
But rather than use a for(;;)
loop you can use a for of
loop. Thus line 15 becomes for(const item of data) {
where item
is each item data. It's a const
as it is a new variable each iteration.
Lines 17,18 is a very poorly written. First line 18 should be indented one tab as it is a continuation of line 17.
The query $("#header")
is very slow and you get the same element each time. Before the loop you should have found the element and stored it in a const
saving heaps of time.
You append HTML to the page which means the browser must parse it, then do a reflow, for every item, so slow. It can be done much faster. See examples
Lines 21,22,23 (and line 9) There is a shorthand way of writing functions, called arrow functions. They help reduce code noise making the code smaller and easier to read.
They differ a little from standard functions but for now you can just change lines 21-23 to reader.onerror = ()=> alert('Unable to read ' + file.fileName);
1 document.getElementById('txtFileUpload').addEventListener('change', upload, false);
2
3 function upload(evt) {
4
5 var data = null;
6 var file = evt.target.files[0];
7 var reader = new FileReader();
8 reader.readAsText(file);
9 reader.onload = function(event) {
10 var csvData = event.target.result;
11 var data = Papa.parse(csvData, {header : false});
12 console.log(data.data);
13 var arrayLength = data.data.length;
14 console.log(arrayLength);
15 for (var i = 0; i < arrayLength; i++) {
16 console.log(data.data[i]);
17 $("#header").append("<li>" +
18 JSON.stringify(data.data[i]) + "</li>");
19 }
20 };
21 reader.onerror = function() {
22 alert('Unable to read ' + file.fileName);
23 };
24 }
Rewrite
The first version is the simplest one (incase you have not used promises before) but not perfect. The role of loadCSV
is not quite just loading as it passes it on to displayCSV
. But would require callbacks to do properly so it will do as is.
Both example don't need jQuery
const uploadInputEl = document.getElementById('txtFileUpload')
uploadInputEl.addEventListener('change',event => { loadCSV(event.target.files[0]) });
// The following function would normally be part of some help utilities like jQuery
const createTag = type => document.createElement(type);
function loadCSV(file) {
const reader = new FileReader();
reader.readAsText(file);
reader.onload = e => displayCSV(Papa.parse(e.target.result, {header : false}).data);
}
function displayCSV(data) {
const list = createTag("ul");
for (const item of data) {
list.appendChild(
object.assign(
createTag("li"), {textContent : JSON.stringify(item)}
));
}
document.getElementById("CSVContainer").appendChild(list);
}
The next uses promises to help keep the function roles succinct. LoadCSV
only loads the data, and displayCSV
creates the displayable content, that goes back to the event that started it all to place where it is wanted.
const uploadInput = document.getElementById('txtFileUpload');
const CSVContainer = document.getElementById("CSVContainer");
uploadInput.addEventListener("change",fileListener);
function fileListener(event) {
loadCSV(event.target.files[0])
.then(data => displayCSV(data)
.then(list => CSVContainer.appendChild(list))
);
}
function loadCSV(file) {
return new Promise(loaded => {
const reader = new FileReader();
reader.readAsText(file);
reader.onload = e => loaded(Papa.parse(e.target.result, {header : false}).data);
});
}
function displayCSV(data) {
const list = tag("ul");
for (const item of data) {
list.appendChild(tagText("li", JSON.stringify(item)));
}
return Promise.resolve(list);
}
// The next two would be part of a utils lib and beyond the scope of the answer
// They do not represent well written code.
const tag = type => document.createElement(type);
const tagText = (type, text) => (type = tag(type), type.textContent = text, type);