Add read.js #2
Add read.js #2
Conversation
| Let's just pretend I named the branch correctly. This is what happens when you commit on Friday afternoon. |
read.js Outdated
| | ||
| // Find up to 5 listings with at least 4 bedrooms and at least 2 bathrooms | ||
| // If you recently ran create.js, a listing named Beautiful Beach House should be included in the results | ||
| await findListingsWithMinimumBedroomsBathroomsAndMostRecentReviews(client, 4, 2, 5); |
There was a problem hiding this comment.
This works fine for the example, but it is approaching a readability issue. This could go either way, as there isn't a preset rule to this, but I would probably rewrite this to use destructuring:
async function findListingsWithMinimumBedroomsBathroomsAndMostRecentReviews(client, { minimumNumberOfBedrooms=0, minimumNumberOfBathrooms=0, maximumNumberOfResults=Number.MAX_SAFE_INTEGER }={}) {Then, when the function is invoked, it would be called by doing:
await findListingsWithMinimumBedroomsBathroomsAndMostRecentReviews(client, { minimumNumberOfBedrooms: 4, minimumNumberOfBathrooms: 2, maximumNumberOfResults: 5 });Pros:
- Makes it clear what parameters you are passing in
- Allows for flexibility in passing parameters (you don't need to worry about order)
- Easier to make parameters optional / have default values
Cons:
- More Verbose
- If you don't want to have any parameters be optional, it makes it a little harder to enforce that
See this stackoverflow question for more discussion on the subject.
There was a problem hiding this comment.
I would also move to passing the collection rather than the client.
There was a problem hiding this comment.
Thanks for the tips on destructuring @daprahamian! I've updated the code. Is it ok/normal to have some functions with desctructuring and some without?
@codepope In the original version of the code, I passed the collection. I had to switch to passing the client when it was decided that the first blog post had to list the databases instead of running a query. To prevent having to re-explain the main() function in every post, I've been passing the client.
read.js Outdated
| // Print the results | ||
| if (results.length > 0) { | ||
| console.log(`Found listing(s) with at least ${minimumNumberOfBedrooms} bedrooms and ${minimumNumberOfBathrooms} bathrooms:`); | ||
| results.forEach(function (result, i) { |
There was a problem hiding this comment.
nit: use an arrow function here:
results.forEach((result, i) => { read.js Outdated
| */ | ||
| async function findListingByName(client, nameOfListing) { | ||
| // See http://bit.ly/Node_findOne for the findOne() docs | ||
| result = await client.db("sample_airbnb").collection("listingsAndReviews").findOne({ name: nameOfListing }); |
read.js Outdated
| // Get the results and send them to an array | ||
| | ||
| // See http://bit.ly/Node_find for the find() docs | ||
| results = await client.db("sample_airbnb").collection("listingsAndReviews") |
ljhaywar force-pushed the create branch 4 times, most recently from b85200d to b309471 Compare 
daprahamian left a comment
daprahamian left a comment There was a problem hiding this comment.
LGTM w/ one take-or-leave-it comment.
| | ||
| // Find up to 5 listings with at least 4 bedrooms and at least 2 bathrooms | ||
| // If you recently ran create.js, a listing named Beautiful Beach House should be included in the results | ||
| await findListingsWithMinimumBedroomsBathroomsAndMostRecentReviews(client, { |
There was a problem hiding this comment.
take or leave it: since you don't need to be explicit with the positional parameters, might want to rename this to something shorter like findListingsAndFilter?
No description provided.