[IGSTK-Developers] String Events vs. Get Methods

[Kevin Gary]

Hey All,

I have to agree with David here. This seems like an awful
lot of overkill just to call a Get method. I agree with
all of David's reasons, and have a few others.

Effectively the patient name is "returned" to the caller
via an event. This creates an awkward situation where you
cannot use a locally scoped variable to store the patient
name, but instead must provide a class member variable the
callback can set to store that value. Then when the call
stack unwinds back to the next line after the original
RequestPatientName invocation in the app, the app can
retrieve the patient name from the class member variable.
To me, it is bad OO practice to introduce class-level
attributes that are not part of an object's state, but
are added artifically to support awkward call sequences
like this.

What happens when a m_GetPatientnameInfoInput does not
map to a transition (I only see one transition for it,
in the image read state). If the SM is in a different
state, won't this just drop out, never sending an
event, and leave the original caller (the app) left to
fend for itself? Would the original app ever know the
callback was never called and therefore its class member
variable storing the result is not the result of the
original request? David's email suggests there is an
error event in this scenario but I do not see it...

I would think it would be very hard to convert this type
of application to be multithreaded if so desired in the
future.

David is correct, the code becomes very verbose. Worse, it
becomes very verbose in the application. I cannot imagine
folks would be excited to have to write code this way to
use IGSTK. While I agree that programmer convenience is
second to program safety, I fail to see how this model is
safer. In fact, I'd say it is less safe. As the generator
of an event is decoupled from the target, what happens here
if there are extra observers registered with the generating
object. This type of Get method clearly is performing a
service on behalf of a specific caller.

I guess I just have a conceptual block on the utility of
this sequence. I have attached a sequence diagram of the
invocation sequence, and it just looks awkward to me. A
Get method means I want to go read/retrieve an attribute
value of some object, not "I want to go ask an object to
read its value and tell all observers what that value is".

Note I do not have an issue with leveraging the state
machine of DICOMImageReader, as this is appropriate. The
patient name value should only be available to be read
if the reader is in a stable state where the value is
available. I just think we should be able to return that
value back to the original requestor. While I know this
may cause an "if" test on the return value, I don't see
that as less safe than rigging observers and callbacks
and obfuscating class state variables to achieve the
equivalent result.

Finally, adding transitions for Get methods like these
that are effectively readers and not modifiers of object
state will inhibit our ability to extend state machines
to include guards, entry/exit actions, or other mechanisms
if so desired in the future. A Get method should be
implemented as an in-state operation (action), or as an
internal transition (a special type of transition that
indicates object state will not change and entry/exits/guards
are not needed - right now we just "const").

Kevin


David Gobbi wrote:
> Hi Everyone,
> 
> This came up at the last t-con, and I thought that rather
> than letting it eat up too much time at the next t-con,
> it would be better to try to get it at least partially
> resolved on the mailing list.
> 
> Right now, the application get the patient name and other
> info from the DICOMImageReader by calling a "Request"
> method, after which the reader will send either a StringEvent
> that contains the info, or an InvalidRequestEvent.
> 
> There has to be application code to do the following:
> 1) set an observer for DICOMPatientNameEvent, and create a callback 
> object for the observer
> 2) set an observer for InvalidRequestEvent, and create a callback object 
> for the observer
> 3) define a callback function (called when the request is successful)
> 4) define an error function (called when the request failed)
> 5) perform the request
> 
> I have three issues with this.
> One is that a large amount of code is required to do a very
> simple thing, all just to make sure that the patient name is
> not requested before the file is successfully read.
> 
> The second is that the code is separated into three separate
> chunks (set observers, define callbacks, perform request) which
> makes it more difficult for a programmer (or reviewer) to look
> at the code and know whether it is doing the right thing.
> 
> Third, if there is a lot of information that is needed from
> the image (patient, patient ID, physician, hospital, date,
> default window/level, modality, scan type) then these issues are
> multiplied by N.
> 
> 
> Personally, I think that the logical place for error checking
> is at the point in the code where the file is read.  We can
> trust that the application developer can check this, and if
> we don't trust the application developer, then we need an
> application "state machine" that does these checks.
> 
> It should be fine if we simply have GetPatientName() etc.
> methods that return a suitable default (e.g. an empty string)
> if the file was not successfully read.
> 
> - David
> _______________________________________________
> IGSTK-Developers mailing list
> IGSTK-Developers at public.kitware.com
> http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers

-- 
===
Kevin A. Gary, Ph.D.
Assistant Professor
DCST, ASU East
(480)727-1373
http://kgary2.east.asu.edu
kgary at asu.edu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DICOMImageReader-RequestPatientnameInfo.pdf
Type: application/pdf
Size: 39179 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/igstk-developers/attachments/20051006/6c35c20f/attachment.pdf>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kgary.vcf
Type: text/x-vcard
Size: 369 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/igstk-developers/attachments/20051006/6c35c20f/attachment.vcf>