Comments on Problem Set 1

Documenting Unit Tests

Comment the module that passed the tests, NOT the test that the modules passed. The test should be an unchanging piece of code, so it should not get edited every time a module that passes the test is written. The module depends on the test, not the other way around. Thus, the module should contain the comment. (Further, sometimes you may not have access to the source code for the test, in which case this point is moot.)

Put the comment about which tests were passed at the top of the class that passes the test suite instead of buried within your code. This makes it easier for other developers to find this information.

The comment should specify which tests were passed (and ideally, when the test was run and by whom, if it is a team project). The comment "passed all tests" is not as helpful to other developers as the comment: "ps1.cards.Deck passed all tests in ps1.cards.DeckTest on 2/12/04."

Formatting Your Code

Java is not a whitespace-delimited language, so please do not treat it like one, even though such syntax may compile. Thus, instead of this:

if (valComp < 0)
  return true;
else if (valComp > 0)
  return false;
else
  return suitComp;

Do this:

if (valComp < 0) {
  return true;
} else if (valComp > 0) {
  return false;
} else {
  return suitComp;
}

In practice, the former is more error-prone, so please avoid it.

Some of you have indentation issues – just remember that every new block should be indented. Thus, instead of:

if (!cards.contains(c)) {
cards.add(c);
}

Do this:

if (!cards.contains(c)) {
  cards.add(c);
}

You can format your code in Eclipse by pressing Ctrl-Shift-F. Please make sure that your code is formatted before submitting it.

Restrict the width of every line to 80 characters. If you use Ctrl-Shift-F in Eclipse, it should take care of this for you. This is an old standard, dating back to times when terminals were only 80 characters wide; however, it is still honored today for various reasons. One of which is that it makes your code easier to read when I print it out!

Performance Issues

Use an Iterator to loop over the elements in a Collection instead of a for loop that uses get(i). When your Collection is a LinkedList, looping over your Collection using get(i) will be quadratic in performance whereas looping over it using an Iterator will be linear in performance.

Minimize the number of casts and calls to instanceof in your code. Both of these operations are relatively expensive, so try to avoid them as much as possible. Instead of this:

public int compareTo(Object o) {
  int valComp = this.getValue().compareTo(((Card)o).getValue());
  int suitComp = this.getSuit().compareTo(((Card)o).getSuit());
  // other stuff...
}

Do this:

public int compareTo(Object o) {
  Card c = (Card)o;
  int valComp = this.getValue().compareTo(c.getValue());
  int suitComp = this.getSuit().compareTo(c.getSuit());
  // other stuff...
}

This improves performance as well as readability.

Avoid redundant code. In the table below, verbose code that I saw in problem sets is on the left with its simplified equivalent on the right:

if (containsObj) { return true; } else { return false; }	return containsObj;
if (!(obj instanceof Card)) { throw new ClassCastException(); } Card c = (Card)obj;	Card c = (Card)obj; // may throw ClassCastException
if (this.compareTo(c) == 0) return 0; else if (this.compareTo(c) < 0) return -1; else if (this.compareTo(c) > 0) return 1; else throw new Exception();	return this.compareTo(c);

Writing an equals() Method for Card

Using compareTo to help implement equals is not the best design decision, although the reason is very subtle. The equals method is special in Java – its specification is explicit and somewhat tricky, but it is extremely important that it is met. Thus, think of implementing equals as being the first priority when implementing Card and implementing Comparable as being second priority. If Card no longer implements Comparable, it must still meet the contract for equals, so equals should be developed independently of compareTo. A good equals method for Card is shown below:

public void equals(Object obj) {
  if (!(obj instanceof Card)) return false;
  Card c = (Card)obj;
  return getValue().equals(c.getValue()) &&
         getSuit().equals(c.getSuit());
}

What the final Keyword Means

Declaring a class final does not mean that it is immutable; instead, it means that no class can subclass that class. Classes are often declared final for security reasons (so they are not overridden maliciously), such as String and Integer. There is no keyword in Java to declare a class as immutable – the best you can do is implement your class so that it is immutable and include a note in the class's Javadoc comment that says it is immutable.

If you declare a method final, it means that it cannot be overwritten by a subclass.

Also, you can declare fields of a class as final, but this does not mean that they are immutable. For example, if you have:

private final HashMap map = new HashMap();

That does not mean that map is immutable. Instead, it means that map cannot be reassigned to another HashMap. However, you can still mutate map with a call to map.put().

Avoiding Rep Exposure

Some of you tried to check if a Card was an ace by checking if its name was "Ace"; however, that would break if its name were changed to "ACE". Instead, test if a card is an ace by doing:

boolean isAce = (card.getValue() == CardValue.ACE);

How to Do listCardsAcesLow() Efficiently

Many of you agonized over how to implement listCardsAcesLow() efficiently. Here is what is likely the most succinct implementation:

public Iterator listCardsAcesLow() {
  CardSuit lowSuit = 
   (CardSuit)CardSuit.SUITS.get(CardSuit.SUITS.size() - 1);
  Card lowAce = new Card(CardValue.ACE, lowSuit);
  SortedSet aces = cards.tailSet(lowAce);
  SortedSet nonAces = cards.headSet(lowAce);
  List acesLow = new ArrayList(aces);
  acesLow.addAll(nonAces);
  return acesLow.iterator();
}

Note that lowSuit is created in a way that it will still work if the order of the suits changes.