Comments on Problem Set 3

Write the Test Cases First

Okay, just because we made it the first problem did not force you to write your test cases first, but you should have. Many people had bugs because they didn't read the specs thoroughly and just went on to write their implementation. Then, they wrote their tests that were based on the impression that they had of specs from writing their implementation. Often times, this impression was incorrect, and resulted in incomplete (or worse, incorrect) test suites that allowed for bugs in your code and flunked the staff's correct code.

Had you written the tests first, then you would have forced yourself to read the specs thoroughly. You would have written better test cases and then would have implemented your code with these tests in mind. If you want to save yourself time on these problem sets, the place where you can save the most time is in debugging. Write your tests first and time will be saved, or your money back.

How to test an Iterator

Write a Worthwhile Rep Invariant

The goal of a rep invariant is to help you find bugs in your code by alerting you to when the representation of your object has been violated. Thus, if your RI for Route was "has at least one segment," then how many bugs do you think that RI will help you catch? If your Route is modified in such a way that immutability is violated or the sequence of RoadSegment objects in your list is changed, then do you think that this RI will help you catch those errors? Probably not. Write an RI that actually tests if the representation remains invariant to help yourself.

Make Sure Car Tests for Behavioral Equivalence

A Car is behaviorally equivalent to another Car if and only if the reference points to the same Car. Many of you wrote an equals() method for Car that tested for observational equivalence instead. As we discussed in recitation, if you have two Cars that are initialized with the same fields, you can change the speed of one of them and then distinguish one from the other by its getSpeed method. Thus, two such Cars would not be behaviorally equivalent.

Change in addSegment Spec is Trivial

Some of you thought that the new spec for addSegment in Problem 3 would require a lot of changes in the implementation. It wouldn't. The only think that you, the implementor, would have to do is check and see if you would have to use the new segment as given, or if you would have to call reverse() on it to get it in its correct orientation before appending it to the end of Route. Nothing else in Route would have to change.

When Are Routes Equal?

Many of you implemented the equals method in Route correctly but got the question wrong in Problem 3. Two Route objects are equal if and only if they have the same segments in the same order. Thus, equivlance for Route is more than them just having the same segments. Consider the code below:

GeoPoint a = new GeoPoint(0,0), b = new GeoPoint(0,1), c = new GeoPoint(0,3);
RoadSegment rs1 = new RoadSegment("rs1", a, b);
RoadSegment rs2 = new RoadSegment("rs2", b, a);
Route x = new Route(rs1);
x = x.addSegment(rs2);
Route y = new Route(rs2);
y = y.addSegment(rs1);
System.out.println(x.equals(y)); // this should be false!

Don't Use clone()

I didn't expect you to know this, but in general the clone() method in Java is a nightmare and you should avoid it. For a justified explanation of why this is, see Item 10 in Effective Java. You could easily avoid cloning lists as follows:

// bad
List list; // some existing list
List copy = (List)list.clone();

// good
List list; // some existing list
List copy = new ArrayList();
copy.addAll(list);

Abstract Out Magic Numbers

I saw the use of 1000000 and Math.pow(10, 6) in many of your methods. Using the first is a poor choice because you could easily have a typo where you are off by one zero. Using the second is also a poor choice because it requires Java to do the computation every time the method is used. The solution is to abstract this number out as a constant in the class where it is used:

private static final int ONE_MILLION = Math.pow(10,6);

This is much more readable and is less error-prone.

Consider Caching Values

In immutable classes, such as GeoPoint, RoadSegment, and Route, it is a good idea to calculate your derived fields once and return the computed value when the same observer is called again. This can save a lot of computation which will become important in Problem Set 5. The return values of the following methods need only be calculated once:

GeoPoint.hashCode()
GeoPoint.toString()

RoadSegment.getHeading()
RoadSegment.getLength()
RoadSegment.hashCode()
RoadSegment.reverse()
RoadSegment.toString()

Route.getEnd()
Route.getFirst()
Route.getLast()
Route.getLength()
Route.getStart()
Route.getSteps()
Route.hashCode()
Route.toString()

Note that you do not have to create the cached value in the constructor of each class; instead, you may want to consider lazy evaluation where you wait until the method is called for the first time before calculating the value, and then you can return the cached value on subsequent calls. For relatively expensive, frequently invoked methods such as hashCode(), this can bring an enormous savings.