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?
