3
\$\begingroup\$

I've learn to code almost alone, so I really need some ideas/advice, as it started to get really difficult to advance.

Because this is mostly server side, I can't provide a running snippet, but yet it's a short snippet.

Code

When I press a UI button, it fetches a resource, and this should create a set of list items. Simple enough:

const btn = document.getElementById("btn")
const ul = document.getElementById("ul")
const putItems = (items)=>{items.forEach(item=>ul.innerHTML=`
<li>${item.msg}</li>`)}

btn.addEventListener("click", ()=>{
    fetch("https://localhost:3000/todos")
        .then(res=>res.json())
        .then(d=>putItems(d))
        .catch(e=>console.log(e))
})

On the server side, the main function and imports are:

const { MongoClient } = require("mongodb")
const cors = require('cors')
app.use(cors())

async function callDB(){   //get data from mongoDB
    const call = await MongoClient.connect( uri, {useNewUrlParser:true, useUnifiedTopology:true})
    let result
    try{
        result = await call
            .db("todos")
            .collection("items")
            .find({})
            .toArray()
    } catch(e){
        console.log(e)
    }

    return result;
}

app.get("/todos", cors(), (req, res)=>{
    const items = callDB().then(d=>res.json(d)).catch(e=>console.log(e))
});

This code imports dotenv to hide my sensible parts, and uses express I removed those imports just to be concise.

Mainly, I'd like to know if my asynchronous practice makes sense, or not. And I'm also getting a CORS error, that couldn't fix after an hour. That would help too.

\$\endgroup\$
3
  • \$\begingroup\$ If this code is not working, then it does not belong on codereview. It would be more appropriate on stakcoverflow. \$\endgroup\$
    – jfriend00
    Nov 27 '20 at 15:53
  • \$\begingroup\$ The putItems() code does not seem to do the right thing. It runs a loop, but only the last iteration of the loop ever ends up in the HTML as each successive iteration of the loop just replace the previous HTML. \$\endgroup\$
    – jfriend00
    Nov 27 '20 at 16:09
  • \$\begingroup\$ @jfriend00 right. it needs += thanks. \$\endgroup\$
    – Minsky
    Nov 27 '20 at 16:12
3
\$\begingroup\$

There is a problem regarding error handling: in callDB() you catch all exceptions and log them in the console without passing them on to the caller. Either you should rethrow the exception (or a modified version that is more helpful for the caller) or not catch it at all. Since the MongoClient.connect() can also fail, I would move it into the try/catch as well:

async function getDataFromMongoDB() {  // renamed so you don't need the comment
    try{
        const connection = await MongoClient.connect( uri, {
            useNewUrlParser: true,
            useUnifiedTopology: true
        });
        return await connection
            .db("todos")
            .collection("items")
            .find({})
            .toArray();
    } catch(e) {
        console.log(e);
        throw e; // or throw something else
    }
}

This would be even more concise if you would extract the creation of the connection into its own function. However, I think you would like to write the whole function in one expression after the return, you would have to write it like this:

return await (await getConnection()).db("todos")...

The first await is required to keep error handling in this function.

The request handler can also make use of async/await:

app.get("/todos", cors(), async (req, res)=> {
    try {
        const response = await callDB();
        res.json(response);
    }
    catch (e) {
        console.log(e);
    }
});
\$\endgroup\$
8
  • \$\begingroup\$ Why return await await ...? \$\endgroup\$
    – jfriend00
    Nov 27 '20 at 3:23
  • 1
    \$\begingroup\$ It's actually not await await but written in two statements it would be let c = await getConnection(); return await c.db(...)...;I just substituted the c in the second statement with the right hand side expression of the first. If you would omit the await in the parentheses, you would call db() on a promise. \$\endgroup\$
    – lex82
    Nov 27 '20 at 6:55
  • \$\begingroup\$ But, there's no point at all to return await c.db(...). That works the same as return c.db(). \$\endgroup\$
    – jfriend00
    Nov 27 '20 at 7:05
  • \$\begingroup\$ thanks. is the 1st function correct? .then(d=>putItems(d)).catch(e=>console.log(e)) I'm not sure what is the importance of the catch there @jfriend00 \$\endgroup\$
    – Minsky
    Nov 27 '20 at 14:40
  • 1
    \$\begingroup\$ @jfriend00 I'm afraid that's not correct. As I pointed out in my answer you need the await to have errors handled in the catch block. If you omit it, an unresolved promise will be returned that bypasses the error handling in the function. You can try it out here jsfiddle.net/4zj7u6yw \$\endgroup\$
    – lex82
    Nov 27 '20 at 15:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.