CSV-simplified: A small open source project

Question

I'm working on a small open source project called CSV-simplified for a course I'm taking (Bloc), and I'm looking for some feedback and potential collaborators. I don't actually know an programmers, so I'm hoping someone here may be interested in helping out.

The basic idea of CSV-simplified is a browser app capable of reading a CSV file and displaying it on the page. I would like to expand upon this by creating an intuitive user interface to allow the user to perform searches and queries on the uploaded file. I'm currently using JavaScript, jQuery, and PapaParse to just display an uploaded file.

I'm looking for any help, suggestions, or feedback on improving and building upon this program. The first thing I'd like to see improved is how the file is displayed, I think editing the JS and adding some CSS could really spruce it up.

Here's the GitHub repo of what's done so far. Here's a Codepen that shows the basic logic that's currently working on the app.

HTML

<input type="file" name="File Upload" id="txtFileUpload" accept=".csv" /> <div id="header"></div>

JavaScript

document.getElementById('txtFileUpload').addEventListener('change', upload, false);
function upload(evt) {
 var data = null;
 var file = evt.target.files[0];
 var reader = new FileReader();
 reader.readAsText(file);
 reader.onload = function(event) {
 var csvData = event.target.result;
 var data = Papa.parse(csvData, {header : false});
 console.log(data.data);
 var arrayLength = data.data.length;
 console.log(arrayLength);
 for (var i = 0; i < arrayLength; i++) {
 console.log(data.data[i]);
 $("#header").append("<li>" + 
 JSON.stringify(data.data[i]) + "</li>");
 }
 };
 reader.onerror = function() {
 alert('Unable to read ' + file.fileName);
 };
}

Answer 1

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);

• conkytom: Thanks so much for taking the time to look this over @Blindman67. Your rewrites are really beautifully done, and have given me a lot to think about. I have only read about promises, but I see now this is a great place to use them. Seriously, can't thank you enough for this.