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?
