Re: [Gnumed-devel] re: commits

[Karsten Hilbert]

> ok, I forgot the issue_id and the episode_id parameters need to  be 
> passed, but the parameters are identical in get_first_encounter and 
> get_last_encounter
> so I think it still can be refactored because the body of the two 
> methods are identical except for the exception message
> and the encounter list index.  That was *a bug* (forgetting to pass the 
> parameters on).
> There was no intended change to what ever occurs in get_encounters( ..), 
> and I don' t think the complete refactoring  would make a difference.
Think again:

def get_encounters()
 - gets the list of all encounters for a patient

def get_first_encounter()
 - gets the earliest encounter for the given issues/episodes

def get_last_encounter()
 - likewise for latest

Suppose some code does the following:

 encs = get_encounters()
 encs.sort(by_issue)
 print encs
... many other things ...
 enc = get_first_encounter()

get_first_encounter() will call get_encounters(). get_encounters()
will return the cached encounters. Initially, the cached
encounters were unsorted (sorted by date in your case). They
got sorted by issue outside get_encounters (because they are
passed by reference). If we now rely on the sort order inside
get_encounters we will get the wrong result for
get_first/last_encounter().

Also, even inside get_encounters one could not simply sort
*once* after retrieving all encounters because the call from
get_first_encounter() to get_encounters() may be constrained
by issue/episode. So at the very minimum get_encounters()
would need to make sure that the *constrained* list is sorted
properly. And this list can change from call to call because
callers can request a *selection* out of the cache by
specifying a range of issues and/or encounters.

I do not think we can refactor the way you did. It *could* be
done this way:

get_first/last_encounter
-> calls _get_encounters()
   ->  calls get_encounters()
       and returns sorted results
from which we select [0] or [-1]

Which I am not sure may be what you had intended. I think,
however, the extra method invocation generates more call
overhead than it gains convenience.

Karsten
-- 
GPG key ID E4071346 @ wwwkeys.pgp.net
E167 67FD A291 2BEA 73BD  4537 78B9 A9F9 E407 1346