Re: [Gramps-devel] Cleaning up some code

[Benny M. <ben...@gm...>]

2014/1/14 Doug Blank <dou...@gm...>

> On Tue, Jan 14, 2014 at 5:42 AM, Benny Malengier
> <ben...@gm...> wrote:
> >
> >
> >
> > 2013/12/29 Nick Hall <nic...@ho...>
> >>
> >> On 27/12/13 23:47, Doug Blank wrote:
> >> >> The proposed changes save typing, but I don't see much of an
> advantage.
> >> > Yes, these two are only about saving typing. I think about how often I
> >> > type "_from_handle" and "_from_gramps_id" and how many more times I
> >> > will type them in the future.... But I think "get_person_from_id" and
> >> > just "get_person" are clear and consistent too, so all things being
> >> > equal, why not be efficient?
> >>
> >> If you want to be efficient then we should make a bigger change.
> >> Consider the following code:
> >>
> >> handle = family.get_father_handle()
> >> if handle:
> >>      father = db.get_person_from_handle(handle)
> >>
> >> What we really want is:
> >>
> >> father = family.get_father()
> >
> >
> > But it would be
> >
> > father = family.get_father()
> > if father:
> >     # do what you want to do if father present
>
> Just to be clear, these are the two pieces of code that we are comparing:
>
> Current API:
>
> handle = family.get_father_handle()
> if handle:
>     father = db.get_person_from_handle(handle)
>     if father:
>         # do something with the father
>
> Proposed API:
>
> father = family.get_father()
> if father:
>     # do something with the father
>
> Having to deal with the handle in the current code exposes details
> that are irrelevant and should be hidden. Consider something a bit
> more complicated:
>
> for p in db.iter_people():
>     birth_ref = p.get_birth_ref()
>     if birth_ref:
>         event = db.get_event_from_handle(birth_ref.ref)
>         if event:
>             date = event.get_date_object()
>             if date.get_month() == 3:
>                 count += 1
>
> In a cleaned up API that would become:
>
> for p in db.iter_people():
>     event = p.get_birth_event()
>     if event:
>             date = event.get_date_object()
>             if date.get_month() == 3:
>                 count += 1
>
> Perhaps this is only slightly easier, but requiring programmers to
> deal with handles when they need not is the point of the proposal.
>

Yes, I agree to this. My remarks up to now are only to indicate:

1. better to not change gen.lib, but instead build on top of it, eg in
gen.api.
Then only gen.api objects need the __db or __famtree object needed for
further lookup.
Instead of our db layer adding the __db field, it should be done explicitly
on creation, or will be present if object created via a method. So not the
approach Nick takes in his master branch

2. access to the key is still needed, we call it handle. You need it for
complex logic, to improve efficiency. Eg, for making keys in lists in
listviews, filtering out in filters, recursive relationship calculations,
... it is better to store only handle in memory in a dict, and do lookup
like that, instead of obtain the full object and then constructing a key
again for the storage as key in a dict. So also objects in gen.api would
need a key or handle or id field, that corresponds to handle in gen.lib


>
> > Further, if you only needed to know if a father is present, the code
> became
> > inefficient, as it already retrieved the father.
> > So you would add a family.has_father()
> > But this would not give you the key to obtain the father.
> > So code becomes
> >
> > if family.has_father():
> >    father = family.get_father()
> >
> > In django they get around the inefficiency, by offering always an _id
> field:
> >
> https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.ForeignKey
> >
> > So there, you would have:
> > father = family.father
> > to obtain the father object, and automatically present without defining:
> > father_key = family.father_id
> > which avoids the database lookup.
> >
> > This shows that exposing the keys is actually a very natural thing, and
> > often needed for efficiency.
>
> The proposal would not prevent accessing the handles, but would merely
> add a few methods to eliminate the requirement that you must use them.
> (I couldn't find any examples that would require the use of handles.)
>
> > This example also shows that changing gramps_id into id can have the
> inverse
> > effect of new developers again considering it as a real id as in django,
> > while it is not. handle is our id.
>
> I don't think that calling "id" by the name "gramps id" makes it
> immediately clear that it is just a user-editable text field that
> should be unique, but is not required to be for a couple of uses. Any
> programmer, new and old, need to be explicitly told this. But, ok,
> this is a minor quibble and if people would rather refer to this field
> as "gramps_id", fine. This is a distraction from the major point
> regarding handles.
>

Yes. I would not use id to avoid confusion. Gramps_id is not much better
though, and gedcom_id something that people in 10 years hopefully no longer
use. Perhaps user_id is best, but also a lot of work.

In general, attributes that are too generic, like id, make programming more
complex, as search starts to return to many hits.

Benny

>
> Thanks,
>
> -Doug
>
> > Benny
> >
> >>
> >>
> >> We could also tidy up some of the utility functions.  For example:
> >>
> >> event.get_participants()
> >>
> >> would be nicer than:
> >>
> >> get_participant_from_event(db, event.handle)
> >>
> >> Why do we expose the database handle in the API?
> >>
> >> Perhaps we need to think about a gen.api module?  This could also
> >> incorporate the best ideas in the simple database access module.
> >>
> >> Regards,
> >>
> >>
> >> Nick.
> >>
> >>
> >>
> >>
> ------------------------------------------------------------------------------
> >> Rapidly troubleshoot problems before they affect your business. Most IT
> >> organizations don't have a clear picture of how application performance
> >> affects their revenue. With AppDynamics, you get 100% visibility into
> your
> >> Java,.NET, & PHP application. Start your 15-day FREE TRIAL of
> AppDynamics
> >> Pro!
> >>
> >>
> http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
> >> _______________________________________________
> >> Gramps-devel mailing list
> >> Gra...@li...
> >> https://lists.sourceforge.net/lists/listinfo/gramps-devel
> >
> >
>