ToDoApp with persistence (using JSON)

This is a simple ToDoApp with a CLI that is made useful by adding persistence. A shout out to @Phrancis and @syb0rg for guiding this little project to where it is now.

The structure is rather simple. The basic class is ToDoItem. It is stored in an implementation of the interface ToDoRepository - in InMemoryToDoRepository or ToDoRepositoryWithJSON. Finally, there's the ToDoApp which performs all the command-line processing.

For those who want to test the app, here is a pre-built JAR file (version 0.3). The code is as follows:

ToDoItem.java

```
package ml.cristatus.todo.model;

/**
* This is the basic unit of information in this system. Everything is built
* around this class. This class defines a basic task that has a unique ID
* that is assigned by a repository. The "name" or content of a task can be
* updated freely. But the ID is updated only once by the repository. Every
* task also has a completed boolean flag to indicate its state
* of completion.
*
* @author Subhomoy Haldar
* @version 0.3
*/
public class ToDoItem implements Comparable {

/**
* The unique and immutable ID.
*/
private final long id;
/**
* The modifiable content of the item.
*/
private String name;
/**
* The status of the task/item.
*/
private boolean completed;

/**
* This constructor creates a new ToDoItem with ta unique ID and text.
*
* @param name The name of the task, or its content.
* @param id The unique ID of the item.
*/
public ToDoItem(String name, final long id) {
this.name = name;
this.id = id;
}

/**
* Compares two tasks based on their order of creation. A ToDoItem
* created later will have a higher ID.
*
* @param item The ToDoItem to compare this item with.
* @return -1, 0, or 1 if this item is created earlier, at the same time as
* the argument, or created after the argument, respectively.
*/
@Override

Here are a few observations from me:

ToDoItem.java

/**
 * Set the completion status of this task.
 *
 * @param completed The new status to be set.
 */
public void setCompleted(boolean completed) {
    this.completed = completed;
}

My personal view in methods like this is that instead of making them setters/mutators, you turn them into actions/features. So instead of having a setCompleted that takes a boolean from a client component, I think it would be a bit more sensible to have something like:

public void complete() {
    this.completed = true;
}

This way, you have more control over the internal state of a given class within the context of a given method, while limiting the method's behavior to something that's more specific. It also makes the code a little more traceable in that if there is no requirement that says a completed TodoItem can go back to being non-completed, you don't have to enable such state transition.

I think it's also somehow more "poetic" to have an invocation like task.complete() compared to task.setComplete(true).

InMemoryToDoRepository.java

/**
 * This class does NOT provide a saving mechanism.
 */
@Override
public void save() {
    // no save mechanism here
}

I'm not really sure, but somehow I feel like this can be an instance of a refused bequest, or some sort of breach of contract. I think you have few options here:

• Remove save from ToDoRepository interface, and make it a specialization of ToDoRepositoryWithJSON.

• Extend ToDoRepository to another interface, say, SaveCapableToDoRepository, define the save method in there and have ToDoRepositoryWithJSON implement the specialized interface.

• Remove save from ToDoRepository, define a separate interface, say SavingRepository, with a save method in it, and have ToDoRepositoryWithJSON implement both ToDoRepository and the new one.

If you don't necessarily agree with this, I think you should at least throw an UnsupportedOperationException, like:

@Override
public void save() {
    throw new UnsupportedOperationException("You can't use save here.");
}

Normally, I do this for methods I have yet to implement while I work on specified methods one by one.

StackExchange Code Review Q#129015, answer score: 5