Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

AbstractClass.findOne should return null for not found #128

Closed
pyramation opened this Issue Oct 1, 2012 · 1 comment

Comments

Projects
None yet
2 participants

If you use libraries that do any asynchronous flow control, the findOne method can cause some hard-to-find bugs in your code, because no value is passed into the callback for objects not found.

I suggest that null is passed when objects aren't found, so that there is at least a null object returned.

https://github.com/1602/jugglingdb/blob/master/lib/abstract-class.js#L390

uses:

this.all(params, function (err, collection) {
    if (err || !collection || !collection.length > 0) return cb(err);
    cb(err, collection[0]);
});

but should do:

this.all(params, function (err, collection) {
    if (err) return cb(err);
    if (!collection || !collection.length > 0) return cb(null, null);
    cb(err, collection[0]);
});

If you are interested, my use-case was the async.waterfall using async.js, and calling Model.findOne(ops, callback) as one function in the array of callbacks

Owner

1602 commented Oct 1, 2012

Fair enough. Will amend. Thanks.

On Tue, Oct 2, 2012 at 12:11 AM, Dan Lynch notifications@github.com wrote:

If you use libraries that do any asynchronous flow control, the findOne
method can cause some hard-to-find bugs in your code, because no value is
passed into the callback for objects not found.

I suggest that null is passed when objects aren't found, so that there is
at least a null object returned.

https://github.com/1602/jugglingdb/blob/master/lib/abstract-class.js#L390

uses:

this.all(params, function (err, collection) {
if (err || !collection || !collection.length > 0) return cb(err);
cb(err, collection[0]);
});

but should do:

this.all(params, function (err, collection) {
if (err) return cb(err);
if (!collection || !collection.length > 0) return cb(null, null);
cb(err, collection[0]);
});

If you are interested, my use-case was the async.waterfall using
async.js, and calling Model.findOne(ops, callback) as one function in the
array of callbacks


Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/128.

Thanks,
Anatoliy Chakkaev

@1602 1602 closed this in cefd79d Jan 22, 2013

@anatoliychakkaev anatoliychakkaev pushed a commit to anatoliychakkaev/jugglingdb that referenced this issue Jan 22, 2013

@1602 1602 Update test for #128 c6d7360
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment