Simple class to represent progress

Question

Progress class was written using TDD.

Interface of this class (pseudo-code):

class Progress
 constructor(title, progressLength)
 getTitle() : String
 getLength() : int
 getCompletedSteps() : List<Integer>
 getCompleted() : int
 getUncompleted() : int

Please criticize from the following perspectives:

• OO design
• naming
• cleanliness
• testing

Unit tests (JUnit 3):

public class TestProgress extends TestCase {
 private int progressLength;
 private Integer[] completedSteps;
 private Progress progress;
 private Progress progressWithoutCompletedSteps;
 private String title;
 @Override
 protected void setUp() throws Exception {
 super.setUp();
 title = "Tested Instance";
 progressLength = 200;
 progress = new Progress(title, progressLength);
 completedSteps = new Integer[] { 1, 2, 10, 3 };
 for (Integer step : completedSteps) {
 progress.complete(step);
 }
 progressWithoutCompletedSteps = new Progress("Title", 100);
 }
 public void testConstructorSetsFieldsCorrectly() {
 assertEquals(title, progress.getTitle());
 assertEquals(progressLength, progress.getLength());
 }
 public void testConstructorThrowsExceptionIfTitleIsNull() {
 checkThrowsIllegalArgumentException(new Runnable() {
 @Override
 public void run() {
 String failTitle = null;
 new Progress(failTitle, 100);
 }
 });
 }
 private void checkThrowsIllegalArgumentException(Runnable runnable) {
 try {
 runnable.run();
 fail("Missing exception");
 } catch (IllegalArgumentException e) {
 // all right
 } catch (Exception e) {
 fail("Type of exception should be IllegalArgumentException");
 }
 }
 public void testConstructorThrowsExceptionIfProgressLengthLessThanOne() {
 checkThrowsIllegalArgumentException(new Runnable() {
 @Override
 public void run() {
 int failProgressLength = 0;
 new Progress("Title", failProgressLength);
 }
 });
 }
 public void testGetCompletedSteps() {
 List<Integer> expectedCompletedSteps = Arrays.asList(completedSteps);
 assertEquals(expectedCompletedSteps, progress.getCompletedSteps());
 }
 public void testGetCompletedStepsOfProgressWithoutCompletedSteps() {
 List<Integer> noCompletedSteps = new ArrayList<Integer>();
 assertEquals(noCompletedSteps, progressWithoutCompletedSteps.getCompletedSteps());
 }
 public void testGetCompleted() {
 int expectedCompleted = sumOf(completedSteps);
 assertEquals(expectedCompleted, progress.getCompleted());
 }
 private int sumOf(Integer[] numbers) {
 int sum = 0;
 for (Integer each : numbers) {
 sum += each;
 }
 return sum;
 }
 public void testGetCompletedOfProgressWithoutCompletedSteps() {
 int expectedCompleted = 0;
 assertEquals(expectedCompleted, progressWithoutCompletedSteps.getCompleted());
 }
 public void testGetUncompleted() {
 int expectedUncompleted = progressLength - sumOf(completedSteps);
 assertEquals(expectedUncompleted, progress.getUncompleted());
 }
 public void testGetUncompletedOfProgressWithoutCompletedSteps() {
 int expectedUncompleted = progressWithoutCompletedSteps.getLength();
 assertEquals(expectedUncompleted, progressWithoutCompletedSteps.getUncompleted());
 }
}

Progress class:

public class Progress {
 private final String title;
 private final int length;
 private int completed;
 private List<Integer> completedSteps;
 public Progress(String title, int progressLength) {
 checkArguments(title, progressLength);
 this.title = title;
 this.length = progressLength;
 this.completedSteps = new ArrayList<Integer>();
 this.completed = 0;
 }
 private void checkArguments(String title, int progressLength) {
 if (title == null) {
 throw new IllegalArgumentException("Illegal title: <" + title + ">");
 }
 if (progressLength < 1) {
 throw new IllegalArgumentException("Illegal progress length: <" + progressLength + ">");
 }
 }
 public String getTitle() {
 return title;
 }
 public int getLength() {
 return length;
 }
 public void complete(int lengthToComplete) {
 completedSteps.add(lengthToComplete);
 completed += lengthToComplete;
 }
 public List<Integer> getCompletedSteps() {
 return completedSteps;
 }
 public int getCompleted() {
 return completed;
 }
 public int getUncompleted() {
 return length - completed;
 }
}

UPDATE:

In addition to what is written.

In your opinion, is the following version of the constructor better than the one already shown?

 public Progress(String title, int progressLength) {
 this.title = title;
 this.length = progressLength;
 this.completedSteps = new ArrayList<Integer>();
 this.completed = 0;
 checkInitState();
 }
 private void checkInitState() {
 if (title == null) {
 throw new IllegalArgumentException("Illegal title: <" + title + ">");
 }
 if (progressLength < 1) {
 throw new IllegalArgumentException("Illegal progress length: <" + progressLength + ">");
 }
 }

Or I should not check the correctness of the arguments in the constructor? Maybe it would be better if the obligation to ensure the correctness of the arguments entrust to the client?

Answer 1

It's not clear to me, what is the purpose of this class? Is it a progress tracker for a process with discrete steps (such as for a multi-page survey) or for a progress bar (such as for a filesystem consistency scanner)? In .complete(int lengthToComplete), the first line (completedSteps.add(lengthToComplete)) suggests the former, whereas the second line (completed += lengthToComplete) suggests the latter.

Is the progressLength constructor argument supposed to indicate the maximum number of elements expected for the ArrayList? If so, use it as a size hint when constructing the ArrayList. Also consider just using an int[] array if you know an upper bound for the array length.

I'm not comfortable with the idea of returning an a List<Integer> from .getCompletedSteps(). Since the returned list is mutable, you're giving away the ability for other code to muck with the internal state of this object. It would be better to expose a more minimal interface, such as public boolean isCompleted(int step). The .isCompleted(int) method would mirror .complete(int) nicely, which is an indicator of a better design. It also does not commit you to using a List<Integer> in your implementation; you would be free to change the internal representation to use any suitable data structure in the future.

To avoid ambiguity, I would rename getCompleted() to getCompletedCount(), and getUncompleted() to getIncompleteCount().

If you are going to check that the preconditions are satisfied, do so as early as possible, before you even get a chance to use the possibly illegal parameter values.

If this class is to be used with a GUI, it might be useful to add some event-handling functionality to it: .addProgressListener(...), .removeProgressListener(...). Then, whenever someone calls .complete(), you call .fireProgressEvent() to notify all listeners.