DoctorMcKay/node-steam-tradeoffer-manager

[suggestion] Not found offer in `getOffer` shouldn't return an Error.

sentisso opened this issue · 3 comments

Hello, IMO the getOffer method shouldn't return an error in case the offer wasn't found.

Currently there isn't a rational way to detect, if an offer no longer exists. It is only achievable in a similar fashion:

manager.getOffer(offerId, (err, offer) => {
    if (err && err.message === "No matching offer found") {
        // trade offer wasn't found
        ...
    } else if (offer) {
        // trade offer was found
        ...
    } else {
        // we got an error, maybe retry later?
        ...
    }
})

and I think this is not a good design, because of the error message comparison.

My proposal would be to call callback(null, null); here.
Meaning "no error and no trade offer => the trade offer doesn't exist".

Note: I'm not sure how funny Steam servers are when it comes to returning accurate data about currently active trade offers.
So if it is possible that the !body.response.offer condition is true even though the trade offer does indeed exist and therefore can be retrieved later, then I would leave it as it is now.

Oh lucky number #333 :)

Okay, nevermind, I think this is good design, because the method is suppose to return a specific trade offer.
If it isn't able to find the trade offer, then that's an error scenario => it should return an error.

Better solution to my problem would be to call the getOffers method and filter out the original offerId locally myself.

You don't need to check the error message, just check the eresult property on the Error object.

const TradeOfferManager = require('steam-tradeoffer-manager');
const {EResult} = TradeOfferManager;

let manager = new TradeOfferManager();
// ...

manager.getOffer(badOfferId, (err, result) => {
    if (err) {
        if (err.eresult == EResult.NoMatch) {
            // the offer doesn't exist
        } else {
            console.log(`Something else happened, maybe we have an error code: ${EResult[err.eresult]}`);
        }
        return;
    }
    // the offer does exist
});