Code review comment for lp:~salgado/tomdroid/sync-ui

On Sat, 2010-08-28 at 08:10 +0000, Rodja wrote:
> > On Wed, 2010-08-25 at 20:26 +0000, Rodja wrote:
> > > It is great to get rid of the templates but...
> > >
> > > 1. The tests are not passing if there are already notes in the
> > > database (see two-way-branch for an setUp/tearDown example).
> > >
> >
> > So, IIUC, you suggest that I just reset the database as part of
> > setUp/tearDown? That sounds fine to me, but the sync-ui branch doesn't
> > have LocalStorage (which you use in the two-way-sync branch to reset the
> > DB), so I'd have to do it manually for now. Something like:
> >
> > activity.getContentResolver().delete(Tomdroid.CONTENT_URI, null, null);
> > Preferences.putLong(Preferences.Key.LATEST_SYNC_REVISION, 0);
> >
> > Is that ok with you?
>
> Yes.
>

Cool, I've done that.

> > > 2. Rather then introducing a new member to flag a note as "template" I
> > > would suggest to seach for the template tag each time
> > > isNotebookTemplate() is called. Performance is not an issue, but later
> > > refactorings will be harder with each new member variable.
> >

One issue with the solution you suggest is that, in the current
implementation, we only have access to the tags (which is in the JSON
representation sent by the server) when creating a new Note row in the
database, so I'd have to store all the tags in the DB somehow. Probably
in a custom format as the web sync gives me JSON and the file sync gives
XML.

Does that sound ok to you? Do you have any suggestions as to which
format to use for the tags in the DB? Ideally I think it should be in a
separate table (linked to the Note table) with one column for the key
and another for the value.

> > It's not because of performance that I made it a flag -- it's because I
> > think we should avoid spending processor cycles unnecessarily, to not
> > drain people's batteries.
> >
> > My thinking was that if you have a long list of notes and you're
> > switching back and forth between any note and the notes list, tomdroid
> > would be doing a good job at draining your battery.
>
> Is it? Have you tested? I would expect Android to keep the notes list
> Activity running while showing a Note; and calling onResume when
> switching back. But guessing is never good when talking about
> optimizations.

You're most likely right here. I didn't know this could happen as I
don't know much about Android and this is the first time I write some
code for it.

>
> > To be honest I have
> > no idea how much power it'd spend while doing that, but it's easily
> > avoidable so I thought it was worth it. I'd be OK with dropping the new
> > flag as you suggest, if you feel it's not worth.
>
> Power consumtion is not easily measureable. But when looking on the
> battery usage graph from my Motorola Droid the big consumers are wifi,
> display, Maps and phone. I prefer having a clean code base which is
> easier to change in the future than "pollute" it with optimizations.

Fair enough.