Notes on the CPSC Team Term Project Implementations

I had hoped (this happens frequently!) to comment on each project individually. I don’t have time to do that, so here is a great big compendium of things that I saw

I will write a second document about the self-reflection documents.

Basic Problems

Here is a list of basic coding issues. Some of you are far beyond here; some of you need to really work on these.

Too much `static`

Use of classes with lots of static methods and variables is frequently a symptom of integration nightmares and/or a lack of design.

I was pleased to see some teams making very deliberate use of static variables in mainly non-static code.

Bad names

Naming things well is the single most undervalued coding activity. Well-chosen class and method names are worth a ton of javadoc comments.

Frequently I found myself reading a method or class and thinking this doesn’t work, then realizing that it does work, but I had been mislead as to its purpose by its name.

Naming is hard, and it is worth having teammates review your names. For instance, I found myself using XXXView names for two different purposes: Sometimes a name like BoardView meant that the class had no mutators; other times, it meant that the class was part of a GUI. Avoiding confusing usage like this makes your code easier to understand.

Certainly, get the basics right:

CamelCase class names. No pegButtons! (PegListener would have been much better in this particular case).
Use noun clusters rather than verbal phrases for class names (prefer class BoardEvaluator to class EvaluateBoard).
Verbal phrases are excellent names for methods, particularly void methods.

`package` level protection

I did not see a lot of non-private non-final member variables, but they did occur in a few projects, in particular there was some package-level protection. For variables, there is no need. Provide getters and setters as needed, and make the variables private. Even package-level getter and setter methods are unusual.

More generaily conceiving of objects as great big globs of data that other people use is not particularly OO. Think hard about cohesion.

More advanced comments

Here are some comments aimed more at improving your code than fixing glaring bugs.

Random Number Generation

I learned about java.security.SecureRandom by reading your project code.

SecureRandom is nice in that it avoids the need to supply a seed to get random numbers, but it is probably not what you want. Here are some non-obvious facts about randoom numbers that I did not get a chance to mention in lecture

Random number generators generally should not be local variables.

Indeed, here is a case where you may want to use a singleton pattern, with something like World.getWorld().getRandom(). The reason for this is that
```
Random r = new Random() ; 
int total = r.nextInt() +r.nextInt() +r.nextInt();
```
typically gives a much more “random” result than
```
int total = 0;
total += (new Random()).nextInt(); 
total += (new Random()).nextInt(); 
total += (new Random()).nextInt(); 
```
To say this differently, the .nextxxx methods are designed to be random, the constructors less so. java.Security.SecureRandom is supposed to have a safe constructor in this regard, but this likely means that the constructor is much slower than the .nextxxx methods.
Generally speaking, you do not actually want randomness. In particular, when you are developing and debugging a program, you want to be able to replicate what you just did.

Minimally, you want to record (through a .log file?) the random seed that you used, so that you can replicate where you got to when you discover a bug. There is nothing more frustrating than bugs that only show up “randomly”.

Action Listeners

The point that I am going to make here is also in the textbook (see Programming Tip 10.2). Generally code that looks like extends MyClass implements ActionListener is a design mistake, because it forces every object of MyClass to have the same Listener.

Instead, move the addActionListener code to the level that creates the MyClass object. I frequently have code in a graphics container object that looks something like

public void doSetup() {
   createSubcomponents();
   wireSubcomponents();
   layoutSubcomponents();
   // top level stuff
}

and in wireSubcomponents(), I put code like myClassObject1.addActionLister(/*...*/).

If you want, you can put handler functions in your component.

public class MyClass extends JComponent {
    ...
    public void processActionEvent(ActionEvent ae) { ... }
}

and then you can write

myClassObject1.addActionLister(e -> myClassObject1.processActionEvent(e))`

myClassObject1.addActionLister(myClassObject1::processActionEvent)

when you are wiring things up. Coding as above lets you easily turn the listener off and on under program control.

In terms of Chapter 8 terminology, creating separate Listeners helps decouple code. In terms of MVC, the Listeners probably belong to Control, where as the container classes are likely View.

More ActionListener

ActionListeners are generally cheap objects to build, so you do not need to limit yourself to one. You can also put knowledge in them.

One project had code that looks like

class XX implements ActionListener {
    public void actionPerformed(ActionEvent event)
        { /* tons of code */ }
}

ActionListener onlyOne = new XX() ;
for (...) { pegButton[i].addActionLister(onlyOne) ; }

A different strategy would be to give the XX objects a little more knowledge:

class XX implements ActionListener {
    private int row, column;
    public XX(int r, int c) { row=r; column=c; }
    public void actionPerformed(ActionEvent event)
        { /* two lines of code using row, column */ }
}

for (...) { pegButton[i].addActionLister(new XX(i%4, i/4)) ; }

By having 16 different XX ActionListener objects, each one can be customized to the peg it is attached to.

Prolix Code

I saw multiple instances of long blocks of code that essentially did conversions between String versions of locations (e.g., “A4”) and row column specifications. Frequently, large code blocks could have been replaced by

String location = /* whatever */;
final int /* zero-based */ row = locstion.charAt(0) - 'A';
final int /* zero-based */ col = locstion.charAt(1) - '1';
// actions based on row, col

(If the string location is user input, further validity testing is needed.)

The general maxim here is DRY (Don’t Repeat Yourself). If you find yourself cutting and pasting large blocks of code, figure out a different strategy. These large blocks of repeated code are a maintenance nightmare.

For the sake of completeness, there are multiple ways of doing the opposite conversion. One is

String location = String.format(“%c%d”, (char) ('A'+row), 1+col);

In regard to this particular example, some thoughts occur to me:

There’s something praisewothy here. First get working code, then fix it for style.
There seems to be some particular programming weaknesses around understanding Strings, and chars, and how to compute them. Maybe this is a result of the strike last semester?
Often putting the information in different places would have removed the need for the conversions(in other words, the code, however it was done, hints at a design flaw).

Prolix Code — Winning conditions

There was a huge range of skill levels manifest in code designed to check for winning conditions. I was certainly pleased by the general willingness to search for good solutions. Again, the goal is DRY.

Had we had more time to work on strategy, you would have found it very helpful to be able to write something like:

for(Line aLine:boadPosition.getAllLines())
   {
   int w = line.getWhiteCount() ;
   int b = line.getBlackCount() ;
   // ...
   }

Summary

The projects were well-implemented.
Some folks should review basic character and string manipulation.
Always think really hard about the names of things.
Aim for DRY code.