[Gnumed-devel] Re: commits

[catmat]

Karsten Hilbert wrote:

> > Index: business/gmClinicalRecord.py
> > ===================================================================
> > RCS file: /cvsroot/gnumed/gnumed/gnumed/client/business/gmClinicalRecord.py,v
> > retrieving revision 1.150
> > diff -u -r1.150 gmClinicalRecord.py
> > --- business/gmClinicalRecord.py        27 Oct 2004 12:09:28 -0000      1.150
> > +++ business/gmClinicalRecord.py        17 Dec 2004 12:47:22 -0000
> > @@ -1362,6 +1362,7 @@
> >                                 filtered_encounters = filter(lambda enc: 
> > enc['pk_encounter'] in epi_ids, filtered_encounters)
> >
> >                 return filtered_encounters
> > +
> >         #--------------------------------------------------------               
> >         def get_first_encounter(self, issue_id=None, episode_id=None):
> >                 """
> > @@ -1370,19 +1371,8 @@
> >                         issue_id - First encounter associated health issue
> >                         episode - First encounter associated episode
> >                 """
> > -               h_iss = issue_id
> > -               epis = episode_id
> > -               if issue_id is not None:
> > -                       h_iss = [issue_id]
> > -               if episode_id is not None:
> > -                       epis = [episode_id]
> > -               encounters = self.get_encounters(issues=h_iss, episodes=epis)
> > -               if encounters is None or len(encounters) == 0:
> > -                       _log.Log(gmLog.lErr, 'cannot retrieve first encounter 
> > for episodes [%s], issues [%s] (patient ID [%s])' % (str(episodes), 
> > str(issues), self.id_patient))
> > -                       return None
> > -               # FIXME: this does not scale particularly well
> > -               encounters.sort(lambda x,y: cmp(x['started'], y['started']))
> > -               return encounters[0]
> > +               return  self._get_encounters()[0]
> >    
>
> There is several issues with this approach:
>
> 1) this is getting the *absolute* first/last encounter while
>   the original code constrained that by issue/episode
>
> 2) Doing it this way will get you a call-by-reference result,
>   IOW you will get a reference to the original encounter
>   list, not a copy. Now, although you sorted the encounters
>   after retrieving them this is not guaruanteed to be stable.
>   Other callers getting a reference may have sorted differently.
>   Hence we need to sort *when needed* despite the performance
>   hit. However, the list of encounters isn't going to be
>   particularly large. It'll be in the order of, what, ten to
>   200 or so ?
>
>  
You're saying get_encounters() uses self.__db_cache['encounters'] and 
therefore returns a reference
to the same list for both methods ?

But  the changes are  just a straight refactoring of the original code 
that was there ( see below), so
the  sychronization bug you're concerned with *was already there* ( I 
didn't introduce the sorting).

To fix such a bug , if ever the need was to arise, which currently the 
gui sensibly doesn't , ( e.g. multi-threaded calls),
a straightforward fix is to use a synchronization object like a lock 
shared by the 2 methods, guarding them.
( java just has the keyword synchronized , used in front of any methods  
of classes where the object instances need
to prevent multiple threaded requests from interfering with each other .)

You could also use wxCriticalSection, where the critical section would 
be from the call to get_encounters()
to copying the object reference at  which end of the list is needed,  to 
a local variable.  Then end the critical section
and return the local variable.

With the refactoring (see below), you would only have to do this once , 
instead of twice.

This is really a future bug, when you decide to  multi-thread the 
client, or make the business objects shared amongst
threads of a multi-threaded remote server .




> > +               return self._get_encounters()[-1]
> >    
> see above
>
>  
>
> >                 encounters = self.get_encounters(issues=h_iss, episodes=epis)
> >                 if encounters is None or len(encounters) == 0:
> > -                       _log.Log(gmLog.lErr, 'cannot retrieve last encounter 
> > for episodes [%s], issues [%s]. Patient ID [%s]' %(str(episodes), str(issues), 
> > self.id_patient))                   
> > -                       return None             
> > +                       episodes = epis
> > +                       issues = h_iss
> > +                       _log.Log(gmLog.lErr, 'cannot retrieve first encounter 
> > for episodes [%s], issues [%s] (patient ID [%s])' % (str(episodes), 
> > str(issues), self.id_patient))
> >    
>
> 3) the error message is misleading as it was copied
>   over verbatim
>
> 4) What are those assignments supposed to achieve ?
>  
it's more obvious without the diff. The original code had duplicate 
logic in both methods, so I refactored it so

> #--------------------------------------------------------               
>         def get_first_encounter(self, issue_id=None, episode_id=None):
>                 """
>                         Retrieves first encounter for a particular issue and/or 
> episode
>
>                         issue_id - First encounter associated health issue
>                         episode - First encounter associated episode
>                 """
>                 h_iss = issue_id
>                 epis = episode_id
>                 if issue_id is not None:
>                         h_iss = [issue_id]
>                 if episode_id is not None:
>                         epis = [episode_id]
>                 encounters = self.get_encounters(issues=h_iss, episodes=epis)
>                 if encounters is None or len(encounters) == 0:
>                         _log.Log(gmLog.lErr, 'cannot retrieve first encounter 
> for episodes [%s], issues [%s] (patient ID [%s])' % (str(episodes), 
> str(issues), self.id_patient))
>                         return None
>                 # FIXME: this does not scale particularly well
>                 encounters.sort(lambda x,y: cmp(x['started'], y['started']))
>                 return encounters[0]
>         #--------------------------------------------------------               
>         def get_last_encounter(self, issue_id=None, episode_id=None):
>                 """
>                         Retrieves last encounter for a concrete issue and/or 
> episode
>                         
>                         issue_id - Last encounter associated health issue
>                         episode_id - Last encounter associated episode
>                 """
>                 h_iss = issue_id
>                 epis = episode_id
>                 if issue_id is not None:
>                         h_iss = [issue_id]
>                 if episode_id is not None:
>                         epis = [episode_id]
>                 encounters = self.get_encounters(issues=h_iss, episodes=epis)
>                 if encounters is None or len(encounters) == 0:
>                         _log.Log(gmLog.lErr, 'cannot retrieve last encounter 
> for episodes [%s], issues [%s]. Patient ID [%s]' %(str(episodes), str(issues), 
> self.id_patient))                   
>                         return None             
>                 # FIXME: this does not scale particularly well
>                 encounters.sort(lambda x,y: cmp(x['started'], y['started']))
>                 return encounters[-1]
Became this.

> #--------------------------------------------------------
>         def get_first_encounter(self, issue_id=None, episode_id=None):
>                 """
>                         Retrieves first encounter for a particular 
> issue and/or episode
>
>                         issue_id - First encounter associated health issue
>                         episode - First encounter associated episode
>                 """
>                 return  self._get_encounters()[0]
>
>         #--------------------------------------------------------
>         def get_last_encounter(self, issue_id=None, episode_id=None):
>                 """
>                         Retrieves last encounter for a concrete issue 
> and/or episode
>
>                         issue_id - Last encounter associated health issue
>                         episode_id - Last encounter associated episode
>                 """
>                 return self._get_encounters()[-1]
>
>         #--------------------------------------------------------
>         def _get_encounters(self, issue_id=None, episode_id=None):
>                 h_iss = issue_id
>                 epis = episode_id
>                 if issue_id is not None:
>                         h_iss = [issue_id]
>                 if episode_id is not None:
>                         epis = [episode_id]
>                 encounters = self.get_encounters(issues=h_iss, 
> episodes=epis)   # *1
>                 if encounters is None or len(encounters) == 0:
>                         episodes = epis
>                         issues = h_iss
>                         _log.Log(gmLog.lErr, 'cannot retrieve first 
> encounter for episodes [%s], issues [%s] (patient ID [%s])' % 
> (str(episodes), str(issues), self.id_patient))
>                         return [None]
>                 # FIXME: this does not scale particularly well
>                 encounters.sort(lambda x,y: cmp(x['started'], 
> y['started']))
>                 return encounters

To handle the different error message,  you could  throw the  error  
from the third method, and handle it separately in the first and second,
or add a  string parameter  with "first" or "last"  and change to 
"cannot retrieve %s  encounter for  ..." % (which, episodes, issues ...)

Wrt to episodes and issues,  I'm not sure if parameter assignment in 
python  means a local variable  is created as well
ie. Does  "encounters = self.get_encounters(issues=h_iss, 
episodes=epis)"  create the issues and episodes variables used in the 
error log messages
parameters ?  I remember getting some sort of error  thrown from the log 
messaging call.

if after  the line marked with # *1,  I put

try:
  print issues, episodes
except:
  print "issues and episodes may not be assigned"

Then when I click a newly created health issue  node ,  the error 
message will be printed ,  and my menu won't work,
so it appears parameter assignment doesn't create local variables.

The bug is in the original code , where the exception log call uses 
issues and episodes. It looks clearer, so I just assigned
with episodes = epis ,  instead of substituting episode with epis in the 
exception parameter list.