Session 23

Toward Better Design via Code Review


CS 2530
Intermediate Computing


Reviewing Code for Pousse

an example of my Pousse game in action

Homework 8 confronted us with some interesting design issues. Let's consider candidate implementations for Pousse and PousseFrame to see how we might have resolved them.

Some common design improvements we could make:

Students often worry that they are just are getting by, writing bad code and not really understanding, while other students are producing professional quality work. True, a few students are creating very nice solutions, and a few are writing code with more flaws. But all of us -- me included -- are in the same place: writing code that does some things well and that can be improved. Don't worry about other students' code. Focus on making you code better. And learn from other peoples' code whenever you can.

What about my code...



Good Clean -- and Well-Designed -- Fun

At the end of last session, we saw a variation of Catch that let us hurl my crazy smiling head across the canvas. I hope you enjoyed this program over the weekend while slaving away on Homework 8 deep into the night.

I enjoy letting off a little steam at the instructor's expense as anyone, but we should at least write good code to do it. What is wrong with that program?

I needed a different kind of ball than in my homework solution, one that used an image. I implemented the new kind of ball by extending the existing ball class with some new behavior. Good OO so far.

But then I edited the existing frame class to hack in my new object. I added an instance variable:

    private Image ballImage;
and initialized it to null in the default constructor. Then I added a new constructor to accept an image:
    public CatchFrame( Image im )
    {
      this();
      ballImage = im;
    }

This doesn't seem so bad, though it creates a potential problem: What if code somewhere in the class tries to use ballImage when it holds null? That should at least cause us to pause. It's a symptom of a problem.

One way to fix the problem is to copy the existing CatchFrame class to a new file and add the behavior we want. Now we don't need a default constructor, just the one that accepts an image. Everything else stays the same from the modified CatchFrame.

This eliminates the potential NullPointerException in our edited CatchFrame, and lets us play both versions of the game. But at what cost? We have duplicated an entire class line for line and made only six small changes: the class name, the new instance variable, the new constructor name and argument, the IV initializer, the type of the ball created, and the image argument to its constructor. The rest of the 202 lines of code are identical.

What if want to change how the speed listener works? What if we want to add a display panel at the top of the frame? What if we want to turn the catcher into a catcher's mitt or a jai alai cesta? Every change must be made in two different classes, and made correctly.

After eleven weeks studying OO programming, can't we do better?

Of course we can. A CatchEugeneFrame is a CatchFrame with a new capability, so we should be able to extend the existing class with the new behavior. That's exactly what I did to implement the BallWithImage class.

What behavior do we want to override? Just the kind of ball that is created in the middle of the PitchButtonListener's actionPerformed() method:

    public void actionPerformed( ActionEvent e )
    {
      updateMessage();
      
      int ballRadius = 5;
      int yDirection = (Math.random() < 0.5) ? 1 : -1;
      double changeInY  = 0.5 * DefaultBallSpeed *
                          Math.random() * yDirection;
      
      ball = new BoundedBall( PitcherCenterX - ballRadius,
                              PitcherCenterY - ballRadius,
                              5, ballSpeed, changeInY,
                              CatchFrame.this );
      repaint();
    }

We don't want to duplicate the rest of that method, so we need to create a method that does only what we want to override.



Refactoring the CatchFrame Class

Let's create a new helper method to create the ball. It will be similar to updateMessage(), the message the listener sends to the frame to change the string displayed at the top of the game window.

    protected Ball makeBall( int x, int y, int r, double dx, double dy)
    {
      return new BoundedBall( x, y, r, dx, dy, this );
    }

Notice that, by creating the ball outside the listener, we do not have to "jump scope" to reach the value of this to send as an argument.

Now we replace the new expression in the listener with a makeBall message to the frame.

    ball = makeBall( PitcherCenterX - ballRadius,
                     PitcherCenterY - ballRadius,
                     5, ballSpeed, changeInY );

This change has no effect on what CatchFrame does. It affects only how it does what it does. We should be able to fire up a game of Catch and play with no noticeable changes. And we can!

This is an example of refactoring, a term we first learned about in Session 14. Why would we make a change that doesn't affect what the class does? In the case of our game of catch, it makes the program easier to extend. In a world where changes will cause us to modify and extend our code, a program that is easier to extend is a better program.



Extending the CatchFrame Class

Now we are ready to implement our new frame as a variation of the existing frame. The only method we need to override from CatchFrame is the method that creates the ball:

    private Ball makeBall( int x, int y, int r, double dx, double dy)
    {
      return new BallWithImage( x, y, r, dx, dy, this, ballImage );
    }

Of course, we also need an instance variable and a constructor to manage the image:

    public CatchEugeneFrame( Image im )
    {
      super();
      setTitle( "Make Him Pay!" );
      ballImage = im;
    }

That's all! Now we can fire up a game of CatchEugene and throw my head across the room to our heart's delight. And we can!

... eliminates nearly all the duplication. There is some overhead to writing classes, and we now have two methods with the same name. But that is not much compared to the 200-odd lines of duplication in Version 2, or the clobbering of our original catch game in Version 1.

Notice, too, that this solution neatly eliminates the potential null pointer problem we saw in Version 1. The ballImage now lives in a subclass, so there is no possibility that the code in CatchFrame can try to use it when it is null. The variable is now private inside the only kind of object that uses it, with a class definition that initializes it immediately upon the object's creation. A good design often eliminates several kinds of problem.



The Factory Method Pattern

One of the most common changes to make to a program is to replace one of the objects it uses with a different object. Unfortunately, using new hardcodes a specific class name into the program.

A common solution to this problem is to move the new expression into a method that returns the new instance. This makes it just like new. However, this method can be overridden by a subclass, meaning that the new code can replace one object with another without touching the existing class.

This technique is called a factory method. OO designers use factory methods to write classes that are easier to extend and easier to modify.

... notice. We can now build an application by instantiating a class that inherits most of its behavior, and all of the control. This is a lot like how the AWT. We have just taken a small step toward creating a framework.

... notice. We may still have a problem. The instance variable in the class hardcodes our program to a specific class of objects. Our factory method must produce one of them, an instance of the class or one of its subclasses. Might we want to throw something other than a Ball? What would such an object need to be able to do? (Paint itself. Tell us if it has intersected the target.) ... we may want to factor out an interface.



Wrap Up



Eugene Wallingford ..... wallingf@cs.uni.edu ..... November 7, 2012