PDA

View Full Version : JSONDataModel.loadData error handling



tryanDLS
30 Nov 2006, 11:15 AM
The 'try' needs to move up a few lines so that it catches a bogus data object, which will crash the 'eval'

jack.slocum
30 Nov 2006, 1:43 PM
Ah, that was added recently. I moved it to the 3rd line.

tryanDLS
30 Nov 2006, 1:51 PM
I'm also thinking it would be useful if loadData returned true/false regardless of whether it invoked callbacks. Maybe I'm overthinking, but it would seem to lead to cleaner code if the load result got passed all the way back up, rather than setting flags or wiring loadevents and going to a bunch of separate methods

Given the following mock code, this gets hard to follow pretty quick


function onLogin {
build1();
// do this depending on whether build1 worked
build2();
// do some more conditional stuff
}

function build1() {
create grid 1 components, cm,dm,sm, etch

wire dm.loadException to showerror method

dm.loadData(data1, data1LoadedCB)
}

function data1LoadedCB(o, status) {
if (status)
render grid 1
else
// do nothing
return status
}

function build2() {
create grid 2 components, cm,dm,sm, etc

wire dm.loadException to showerror method

dm.loadData(data2, data2LoadedCB)
}

function data2LoadedCB(o, status) {
if (status)
render grid 2
else
// do nothing
return status
}

function showerror () { alert error1; log error}

jack.slocum
30 Nov 2006, 2:42 PM
I hear ya, but because it is intended to be used async I think it should present a consistent API. If it returned a boolean and had a callback I think some people would be quickly confused. I could of course return an undocumented boolean. :D

tryanDLS
30 Nov 2006, 2:51 PM
It probably wouldn't be good to make this appear different than any of other stuff that follows this pattern. It's something I've had a 'problem' with since I started with the base YUI.Connect.

What about adding a loaded status flag to the DataModel, that way it's contained in that object, rather than events or callbacks having to set a flag someplace else.

jack.slocum
30 Nov 2006, 3:04 PM
That try/catch is killing you, huh?

What about this:

dm.fireLoadException = function(e){ throw e; };

Then you could just use a standard try/catch:


try{
dm.loadData(data1);
// render grid or whatever
}catch(e){
showError(e);
}

tryanDLS
30 Nov 2006, 3:16 PM
Yeah, didn't think of that - that'll work

It's not so much that the other way won't work, but if I need to get other devs involved (who are, shall we politely say, 'less than stellar'), I need to dumb down the API as much as possible. They will at least understand 'wrap the load in a try/catch' :)

jack.slocum
30 Nov 2006, 3:31 PM
Yeah, that's pretty standard. It's too bad the try/catch is in loadData. I think it might make more sense in load().

tryanDLS
30 Nov 2006, 3:56 PM
I'm looking at this some more and in LoadableDataModel.processResponse, it looks like maybe there's an issue with the var used for callback. cb is assigned the repsonse.argument.callback, but it's not used consistently thru out the method. In fact the catch is using 'callback' at the method

jack.slocum
30 Nov 2006, 4:16 PM
Yeah, that looks like a bug. I corrected it.

tryanDLS
30 Nov 2006, 4:30 PM
maybe another in JSONDataModel - it calls callback(true/false). I didn't catch this cuz I call loadData directly, with a calllback method, not an object. If it's not a function, shouldn't it call callback.success or callback.failure?

jack.slocum
30 Nov 2006, 4:37 PM
Nope.


var cb = {
success: this.processResponse,
failure: this.processException,
scope: this,
argument: {callback: callback, insertIndex: insertIndex}
};

and


var cb = response.argument.callback;

So the cb in processResponse is the actual callback passed by the caller.

tryanDLS
30 Nov 2006, 4:45 PM
DOH! Helps to look at the doc - I thought load() took an object or function as a callback.