eliezerwohl / zooApp

0 stars 0 forks source link

Feedback - Week 12 Assignment #18

Open ntuvera opened 8 years ago

ntuvera commented 8 years ago

Hey @eliezerwohl, nice work on the ZooApp. Functionality seems to be all there. I did look over the actual code though and ran into a few questions.

 var trainer = Math.floor(Math.random() * 10);
    // need this or else it won't write, use random number generators 
      connection.query('INSERT INTO animals (name, type, age, caretaker_id) VALUES (?,?,?,?)', [result.name, result.type, result.age, trainer]

Based on the homework instructions I can see why you trainerI think the homework instructions were unclear. But based on your SQL command, I would've added a the option for caretaker_id into my prompt. Either way, this could be a good example of a client not knowing what they want or sometimes a spec needs to be discussed. Either way no points against you, you made it work.

connection.query('select * from animals where type=?', [result.animal_type], function(err, results){ console.log("there are this many of that type of animal:" + results.length)

Is there a specific reason you used results.length vs SELECT COUNT()? Once again it works, but why abstract the data you want, when the SELECT COUNT() returns the value already? You can save yourself one layer of abstraction and show mysql some love.

prompt.get(['yes'], function (err, result) { ... } Any reason why you added the prompt, which doesn't actually verify anything? I'm nitpicking, but that's only because you're doing well.

I guess the last bit in promptUser, You've got your throw err as part of your if/else if/else logic. I think there should be a separation of the if/else if/else logic and error/validation handling. It works as is, but I couldnt think of a way to force an error from the prompt.

eliezerwohl commented 8 years ago

Hey @ntuvera. The reason I The reason I did a .length i it never occurred to me not to. Still used to thinking in a front end frame of mind.

I added the prompt of "yes" because it gave me a chuckle. Probably not the best design choice, but given as every other part of the zoo required an input, it seemed somehow wrong to leave one out.

The reason my throw err are part of If statements is because I was getting an error if they weren't. I honestly can't explain it, but I wanted to make it work.

Thanks for the feedback.

ntuvera commented 8 years ago

In terms of the .length, if you're comfortable accessing properties in the results object via . notation, I'm happy.

For your prompt, design choice is your own, it's just funny because I tried no to see if it would work.

I will try to take a look at your if error bug when I can, still catching up on feedback for other students.

At the end of the day, you're job as a dev will be to deliver functioning code within a deadline. And that you did, very well. Just be ready to defend your choices to clients/managers. Unfortunately clients don't always know what they want, lol. Keep up the good work