Is it bad practice to use a promise without reject?

Question

I have the following;

function isUserInDatabase(serverID, playerID) {
    return new Promise((resolve, reject) => {
            executeQuery("SELECT * FROM playerdata where serverID=" + serverID + " AND playerID=" + playerID).then((res) => {
                if (res[0] === undefined) {
                    resolve(false);
                } else {
                    resolve(true);
                }
            });
    }).catch ((e) => {
        console.error(e);
        console.log("Error retrieving data from database.");
    });
}

But I have no reject call. Is this bad convention?

Edit: I'm honestly not sure if this is better. I've done a bit more reading into promises, and perhaps this is a little better, but I'm not sure.

async function handlePlayer(serverID, playerID) { //TEST
    console.log(await isUserInDatabase(serverID, playerID));
}

function isUserInDatabase(serverID, playerID) {
    return executeQuery("SELECT * FROM playerdata where serverID=? AND playerID=?", [serverID, playerID]).then((res) => {
        if (res[0] === undefined) {
            return false;
        } 
        return true;
    })
    .catch ((err) => {
        console.log(err);
    });
}

async function executeQuery(query, opts) {
    let conn;
    try {
        conn = await pool.getConnection();
        return await conn.query(query, opts);
    } catch (err) {
        console.log(err);
    } finally {
        conn.end();
    }
}

From my point of view: no. Absolutely nothing out there to suggest that it's a bad practice to use a `Promise` without `reject`. — AJC24, Nov 30 '19 at 23:01
Note: you are using the [Promise constructor antipattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it). You should stop doing that. Note also that not using the completely unnecessary construction avoids the question entirely. Also, and most importantly, NOTE that your query is vulnerable to SQL injection! — Jared Smith, Nov 30 '19 at 23:07
I'll have a look into that, thank you @Jared Smith. Additionally, I was aware of SQL Injection, but I had issues with the query string to begin with, so I wanted it simplistic for testing. — , Nov 30 '19 at 23:24
Yes, this is definitely a bad practice - you try to `.catch()` errors but there are none. And worse, there actually could be some that you don't handle properly, due to your usage of the promise constructor antipattern. — Bergi, Dec 01 '19 at 00:11
Thanks. I have made some changes. I'm not 100% sure that I'm avoiding that now, though. — , Dec 01 '19 at 09:12

score 5 · Accepted Answer · edited Dec 01 '19 at 00:14

Well, you can use promises without reject. However you must be sure that resolve is called in all your function flows to trigger the "then" handler (or the function must end with an error or exception to trigger the catch clause). Reject is often used to trigger error handling logic linked to variable values and results. If the only way your code would fail is due to an exception, then reject is not necessary (this is my opinion though)

As @Jared Smith pointed out, you should do things this way, where the promise actually can reject:

function isUserInDatabase(serverID, playerID) {
    const query = "SELECT * FROM playerdata where serverID=" + serverID + " AND playerID=" + playerID;
    return executeQuery(query).then(res => {
        const isUndefined = res[0] === undefined
        return !isUndefined
    }).catch (e => {
        console.error(e);
        console.log("Error retrieving data from database.");
    });
}

Though you will not be able to handle this promise errors (there's already a catch clause)

You could ignore said catch clause and handle errors after calling isUserInDatabase:

function isUserInDatabase(serverID, playerID) {
    const query = "SELECT * FROM playerdata where serverID=" + serverID + " AND playerID=" + playerID;
    return executeQuery(query).then(res => {
        const isUndefined = res[0] === undefined
        return !isUndefined;
    });
}

...

isUserInDatabase("some_server","some_player")
.then( ok => console.log("User is in database!") )
.catch( e => {                
    console.error(e);
    console.log("Error retrieving data from database.");
})

I'm not encouraging it. The question was "Is it bad practice to use a promise without reject?" . I replied within that context. The code snippet I included was to indicate that you can ignore the "reject" param if you want to — Martín Zaragoza, Nov 30 '19 at 23:12
I've edited the solution to avoid the Promise constructor antipattern — Martín Zaragoza, Nov 30 '19 at 23:28

Is it bad practice to use a promise without reject?

1 Answers1