models: Delegate adapter logic to Database & minor improvements

[YagoGG]

A continuation of @theo-damiani's work on #36.

Props to him for pretty much all of it. :)

[YagoGG]

Hey, @theo-damiani! Thanks for the review.

[...] we have to be careful to not assign the same task in multiple to-do lists.

I do not think this is a problem. As I see it, the Task/TodoList APIs are not for external users to use; but there's a "contract" between the different parts of the app that we have to respect.

Thus its fairly reasonable to assume that —unless we have a bug— each Task will be added to a single TodoList.

Maybe add a copy of the task and not the task itself ?

This is something that worries me as well, especially because with this model that we have right now we keep different sources of truth. I will explain this in more detail in my reply to Vincent's review, but hopefully once we have integrated this with the actual task creation logic it will be easier to clean up.

[YagoGG]

Thanks a lot for the feedback, @VPellenc!

If the Task object only had a String as attribute I think it could even have been made immutable, but I think there should be a checkBox boolean attribute as well (together with the ability to update it).

That is correct! The reasoning behind preserving its mutability is exactly that: we expect to add other attributes that can be mutated in the future. I have not added a checked attribute because we still don't have a clear way to alter objects (i.e. to change that attribute and reflect it both in the object and the copy stored in the database), so that's why I would feel more comfortable with waiting until we have this integrated with the view before adding any additional functionalities.

Also, the TodoList object only has a getTask(idx) getter to get a task. Nothing that returns a (unmodifiable?) list. For me that makes it unclear how that information is going to be passed to the view.

You are spot on. To give you an example of how can this go wrong:

Database database = new VolatileDatabase();
TodoList todoList = new TodoList("My list");
Task task = new Task("Do something");

database.putTodoList(todoList);
todoList.addTask(task);

assertEquals(task, todoList.getTask(0));
assertEquals(task, database.getTask(todoList.getId(), 0));  // This breaks

That is why I mentioned in this comment that using an observer for each TodoList might be a good idea: that way, every time the in-memory object is mutated, we let the database know so they stay in sync. And, most importantly: that way we only have one way of doing an operation (instead of the two ones I show above).

So I totally agree with this having some important issues regarding mutability, but I think that we should figure out first what the view is going to consume, and then adapt the Database/TodoList APIs accordingly, because otherwise we are at risk of using a design that is not compatible with the Android view.

I would vote for merging this (functional yet quirky) initial version, create an issue to track what we are discussing, and give it a high priority in the next sprint so we can iterate over it. What do you think?

[VPellenc]

I would vote for merging this (functional yet quirky) initial version, create an issue to track what we are discussing, and give it a high priority in the next sprint so we can iterate over it. What do you think?

That makes sense for me. The patch is very clean in the current state so indeed let's try not to do everything at once

[YagoGG]

Thanks, folks! I updated the PR with the latest master.