2012.3.30 Review (Jimmy Theis)

DatabaseHelper Persistance

Following up in our discussion about using static instances of DatabaseHelper, and in response to the CONSIDER in DatabaseHelper, I think Kevin's Application subclass is actually the proper way to share the instance. This StackOverflow post explains how to do it. I've seen others say that singletons actually don't have any guarantees associated with them in Android (their memory can be reclaimed). I haven't found any proof of this, but if an Application subclass is what is recommended, that's probably the best idea for persisting global instances of things. It also very easily solves the need for a Context problem, because Application is a context. It looks like the only thing you were missing when you tried this before is the Android Manifest entry (I think).

TableAdapter Base Class Complexity

It doesn't matter too much, but I think everything in TableAdapter from Line 96 down is sort of overkill by Kevin. They're little wrappers for retrieving values from Cursors and mechanisms for checking which thread it's running on. I don't use either.

TableAdapter Subclass Item Returns

The way you're returning objects from your TableAdapter subclasses now (passing in a List<?> to populate) is very efficient, because each object is only touched once (for example, loadAllLists() in ShoppingListDataAdapter). I did this for a long time, but didn't really like having to toss a reference of an object to be populated over the wall and trust that it was populated when the function returns (feels like a C function or something). So here's my (inspired by Kevin's) idea for being able to return all retrieved objects without having to touch them all twice:

In each TableAdapter subclass, there is an embedded Iterator class for iterating over the TableAdapter's type

Iterator Superclass (in its own file) (whitespace compressed for length in this page):

public abstract class TableAdapterIterator<T> implements Iterator<T>, Iterable<T> {
    protected Cursor mCursor;

    public TableAdapterIterator(Cursor cursor) {
        mCursor = cursor;
        mCursor.moveToFirst();
    }

    protected Cursor getCursor() { return mCursor; }

    /* This is the only method subclasses need to override */
    protected abstract T getObjectFromNextRow();

    public int getCount() { return mCursor.getCount(); }

    /* Methods to make this an Iterator */
    public boolean hasNext() { return mCursor.getPosition() < mCursor.getCount() - 1; }
    public T next() {
        mCursor.moveToNext();
        return getObjectFromNextRow();
    }
    public void remove() {  throw new UnsupportedOperationException(); }

    /* Method to make this an Iterable */
    @Override
    public Iterator<T> iterator() { return this; }
}

In each TableAdapter subclass, make a public subclass of TableAdapterIterator:

public class SomeTypeTableAdapterIterator extends TableAdapterIterator<SomeType> {

    public SomeTypeTableAdapterIterator(Cursor cursor) {
        super(cursor);
    }

    @Override
    protected SomeType getObjectFromNextRow() {
        SomeType result = new SomeType();

        Cursor cursor = getCursor();

        result.setID(cursor.getLong(cursor.getColumnIndex(ID_COLUMN)));
        // Set additional properties from the cursor's results

        return result;
    }
}

Instead of returning Cursors from methods, just pass the cursor into the new iterator's constructor and return that instead. Now your Activity (controller) code goes from this:

tableAdapter.clearAndPopulateThisListITrustYou(myVeryImportantList);

to this:

myVeryImportantList.clear(); // You now control when the clear happens
for (SomeType o : tableAdapter.getAllObjects()) myVeryImportantList.add(o); // You control the add, too

It's just a suggestion, and I think we've talked about it briefly in our Skype sessions. I think it's a nice abstraction that keeps you from having wonky methods in the TableAdapters that modify collections in place.

DisplayOrder Enum Placement

The DisplayOrder enum can probably actually be placed inside ShoppingList, since it always applies to a ShoppingList anyway. This also makes its name a little more descriptive (it's now the ShoppingList.DisplayOrder type). The same is true for ItemUnitLabel and Item.

Price and Unit Representation

Your use of EPSILON in Item made me Google a preferred solution, which yielded this StackOverflow post. That may be helpful for cleaning up that price comparison.

I don't think you necessarily need to break out a whole object for unit size and label (RE: CONSIDER) as long as you're using an enum for your unit label. If you decide to make a more flexible "make your own label" structure, then that's probably complex enough to warrant its own class. Totally your choice, though. If you do make a new class at this point, it should probably be a class that's internal to Item.

General Style

Not that anyone cares, and there's probably religious wars over it, but Android does have a set of naming conventions that I think are pretty reasonable. You prefix all instance (member) variables with a lowercase "m", so you have things like mCurrentList, etc. I like it, because it removes the need to have this. in front of everything. Just a thought.

Note

Current code length: 600 LOC for DB, 700 LOC for model, 550 LOC for tests, 1850 LOC total, vs. 1700 LOC for whole prototype.