Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finished Assingment. Submitting for feedback. #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sri-go
Copy link

@sri-go sri-go commented Jan 27, 2020

Hi Nathan,

I wanted feedback on the assignment. I did have some question regarding the efficiency of my code. The way I did the conversion of JSON to CSV seems inefficient to me. I essentially hardcoded the locations of the distinct fields into a pre-formatted array, but I realize that this isn't the best way. I was wondering what the other way to do this was. Part of my difficulty was understanding how do I sort the Key-Value pairs into their respective array.

arr[8].push(a[i].WEBSITE);
arr[9].push(a[i].DENTAL_PHONE);
arr[10].push(a[i].FULL_ADDRESS);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did this in a slightly unorthodox way (I'm not going to say it was wrong because I think there's a good argument to be made for representing a CSV just like this).

As for performance: I don't think you should sweat it and I don't think there is much room for improvement. Luckily for us, of the actual interpreter which underwrites modern javascript evaluation is highly optimized down to C (that's what Google's/node.js' V8 engine does).

As for readability, you might prefer something like this (and we might even want to dispense with the loops for readability's sake... more on that in a coming class):

// our header is here
var keys = Object.keys(a)
// create the array of arrays for the body
var body = []
// a bit of setup in anticipation of the loop ahead
for (key in keys) { body.push([]) }
for (var k = 0; k < keys.length in keys; k = k + 1) {
  for (var i = 0; i < a[i].length; i = i + 1) {
    var key = keys[k]
    body[k] = a[i][key]
  }
}
var result = [keys].concat(body)

Copy link
Member

@moradology moradology Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this code works (pretty sure, but I typed it in haste and didn't test it), it should generalize to key-value mappings regardless of the underlying keys or how many there are

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants