Last time, we jammed our new density code into the system, adding some incompatible methods and values. Today we're going to begin to smooth out that code to get the design back closer to good. Will we succeed, or fall on our faces?

Midstream ...

We’re in the middle of converting our code to work in inches. Our concern has been to make this “major refactoring” without breaking the system, and without too much guessing about what we want before we know what we want. To do that, we begin by finding out almost exactly what we want, by implementing the new functionality, then, once that’s done, we remove any old functionality that we no longer want, and consolidate the names. We expect that in the first part of this process, we’ll wind up using some names that are less than ideal, because the “good names” will be taken by code we plan to replace.

An alternative, I suppose, would be to rename the existing methods to weird names so that we can immediately use the better names where we want them. Since we’re feeling our way to the new functionality, we’re used to having names be not quite right, so the extra confusion of renaming things right underneath ourselves doesn’t tempt us. In any case, our mission is to report what we do, and yours is to assess it and take away anything of value that you see.

Our basic plan for today is to look through the code now, and see whether there’s older code, such as for the density spikes, that should be removed, or refactored to work in the new way. Before we do that, though, we’d like to talk about an email we received this morning.

Torbjörn Gyllebring writes to tell us that while he finds the article series to be entertaining and educational, he is concerned about our current path of changing the ShotPattern to work in inches rather than pixels. I’ll not quote him at length here, but here’s a representative quote:

From my point of view your data processing domain, that is most of your computations, have a native pixel based domain, even better it's a domain where all non calculated values are integers and that offers a lot of simplicity. My feeling is that doing the pixels to inches transition in the hit list is missguided simply because it gave rise to the need for mucking about with doubles figuring out epsilons and introducing all sorts of "funny" precision issues associated with floating point maths. If you view inches as belonging more to the presentation I would say most of these issues would get pulled out from the lower layers, that is, instead of forcing your domain into inch-land why not do the conversion when constructing your rectanlges? That would allow you to keep the nice pure integer representation of hits and really don't bother with conversions until you really need to.

Torbjörn raises a valid point, and we talked about it this morning before getting down to coding. Our view is that the ShotPattern is a domain object meant to represent the target sheet of paper, and as such, it’s appropriate that it is dimensioned in user terms, to the extent that it’s dimensioned at all.

And in fact, though we called the notion “inches”, the dimensioning of the ShotPattern is really entirely notional. The creator of the pattern tells the pattern what its dimensions are, and those can be anything, true or not. In fact, in our small pattern test, we told ShotPattern that the bitmap was 4x3, and while it is approximately 4x3 in the ratio of width to height, it is emphatically not representative of 4 by 3 inches in the picture. So we’re not really tied to inches, and we’ll keep that in mind as we refactor.

The pixels being integers, I think, is a tossup. True, integers are easy to work with … except that we’re dealing with quantities that need to be rounded, for the center of mass calculations and the like. It is true that we might not have had to test Rectangle2D for openness, but that’s not a big deal either way.

Torbjörn would have liked to have implemented his own Rectangle object – and so would I. On the other hand, we’ve been being berated by other writers for not reusing enough of the existing Java codebase. We feel like we can’t win here!

Still, Torbjörn raises an interesting topic, which relates back to the old debate about whether OO is best done with “real world” objects or not. Suffice to say that in this instance we feel that we’re on the right track, and we’ll keep an eye out for issues that arise because of this change. We’re grateful to Torbjörn, and to everyone who writes to us.

Digging into the Code

Our mission today, and for the next few sessions, is to continue working on our “major design change”, converting our density calculation to work in user coordinates, which caused us to implement an explicit scale conversion into the ShotPattern and such, last time. We chatted about how to do it for a bit, and then dug in. Our first point of attention was the Hit object, which we had hammered a bit last time. It started out like this:

public class Hit {
    private List<Point> points = new ArrayList<Point>();
    private double r;
    private double theta;
    private int xOffset;
    private int yOffset;

    public Hit(int x, int y) {
        this.addPoint(new Point(x,y));
    }

    public void addPoint(Point point) {
        points.add(point);
    }

    public int getX() {
        return points.get(0).getX();
    }

    public int getY() {
        return points.get(0).getY();
    }

    @Override
    public int hashCode() {
        final int PRIME = 31;
        int result = 1;
        result = PRIME * result + getX();
        result = PRIME * result + getY();
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        final Hit other = (Hit) obj;
        if (getX() != other.getX())
            return false;
        if (getY()!= other.getY())
            return false;
        return true;
    }

    @Override
    public String toString() {
        return "Hit(" + getX() + "," + getY() + ")";
    }

    public double r() {
        r = Math.sqrt(getX()*getX()+getY()*getY());
        return r;
    }

    public double theta() {
        theta = Math.atan2((double)getY(), (double) getX());
        if (theta < 0) theta += 2*Math.PI;
        return theta;
    }

    public void translateBMP(int xOffset, int yOffset) {
        this.xOffset = xOffset;
        this.yOffset = yOffset;
        for (Point point: points)
            point.translateBMP(xOffset, yOffset);
    }

    public double getXinInches(double xPixelsPerInch) {
        int xInPixels = getX() + xOffset;
        return xInPixels / xPixelsPerInch;
    }

    public double getYinInches(double yPixelsPerInch) {
        int yInPixels = yOffset - getY();
        return yInPixels / yPixelsPerInch;
    }
}

The highlighted bits are interesting. We want to remove the fact that the Hit is in absolute coordinates and is then translated to the center of the screen and its y coordinate inverted. We proceeded by commenting out the translateBMP method and the conversions in the get_inInches methods. We first made the existing getX methods private and renamed them, and then changed the get_inInches methods to getX and getY, including a scale factor:

   public double getX(double xPixelsPerInch) {
        return x / xPixelsPerInch;
    }

    public double getY(double yPixelsPerInch) {
        return y / yPixelsPerInch;
    }

We followed our noses and made all the calls to plain getX() and getY() use a conversion factor of 1, which results in the same value that they had before, except untranslated and uninverted. This broke a few JUnit tests because they were returning centered values and we verified their results and changed them:

   @Test
    public void useRasterToCreateShotPatternBiggerFile() {
        ShotPattern  shotPattern = new ShotPattern(folder + "PB270011.bmp");
        assertEquals(new Hit(1139,470), shotPattern.centerOfMass());
    }

        @Test
    public void centerOfSmallPicture() {
        ShotPattern  shotPattern = new ShotPattern(folder + "x4y9on7x12.bmp");
        assertEquals(new Hit(4,9), shotPattern.centerOfMass());
    }

The previous values for those were 115,297, and 1,-3, if you’re keeping score. The unit tests were then green again. We also updated corresponding values in the FitNesse tests, which I’ll show you later.

We were irritated by how many getX() had to be changed to getX(1), and would have reverted that out and done something a little differently, but we weren’t connected to our repository and couldn’t refresh back to our last green easily. (We felt that the local history feature of Eclipse was less help than we’d have liked, and decided to go forward.)

We observed that we have no reason for the Hit object to contain a list of points that have been taken from the bitmap. We may need it later but we don’t need it now. So we changed PatterningSheet not to record that information, moving from this:

   private Hit makeHit(int yOffset, int xOffset, int y, int x) {
        Hit hit = new Hit(x,y);
        List<Point> additionalPoints = additionalPointsInThisHit(x,y);
        for (Point point: additionalPoints)
            hit.addPoint(point);
        hit.translateBMP(xOffset, yOffset);
        return hit;
    }

… to this:

   private Hit makeHit(int yOffset, int xOffset, int y, int x) {
        Hit hit = new Hit(x,y);
        additionalPointsInThisHit(x,y);
        return hit;
    }

We have to still call additionalPointsInThisHit(), to consume all the adjacent bits, but we don’t give them to the Hit object. We expect to extend this object to create the hit directly in scaled coordinates, or perhaps to scale it in ShotPattern, but that’s not done yet.

We think that the adjacent point scanning logic in PatterningSheet is a bit baroque and we made a note to look at it for cleanup later, but we’re on a different mission now, so we didn’t digress. We made the obvious changes here, changing the additionalPoints method to void and removing its return. We made a brief foray into having our Point object contain a real and decided that was unnecessary, so we backed it out. Otherwise, in Point, we removed the translateBMP method, as well as the other occurrences of the method of the same name, both implementation and usage.

All this time, by the way, we made a simple change, followed the Eclipse red X’s to fix the compile, and reran the tests, which were all green after those initial changes to the center of mass values. We were green all morning, about every ten minutes.

ShotPattern got all its getX() and getY() converted to getX(1) and getY(1). (We should have made getX() with no parms a method in Hit, but didn’t think of it until too late. No harm done, we’ll probably fix it later.) We then returned to Hit class. We removed the Point list, and the offsets, and gave the class double x and y fields. (This was probably premature, but it was harmless.)

public class Hit {
    double x;
    double y;
    private double r;
    private double theta;

We changed the constructor on x,y not to create the first list Point, but instead to store the x and y:

   public Hit(double x, double y) {
        this.x = x;
        this.y = y;
    }

We removed addPoint, which was no longer called from the PatterningSheet’s getHits() method.

We removed the baroque getX() and getY() methods, which were no longer used because everyone was calling the new ones with the scaling factors, as shown before:

   public double getX(double xPixelsPerInch) {
        return x / xPixelsPerInch;
    }

    public double getY(double yPixelsPerInch) {
        return y / yPixelsPerInch;
    }

We fiddled the hashcode and equals methods to cast the doubles to int, to use the new get (done a bit earlier) and to access the local values of x and y directly:

   @Override
    public int hashCode() {
        final int PRIME = 31;
        int result = 1;
        result = PRIME * result + (int)x;
        result = PRIME * result + (int)y;
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        final Hit other = (Hit) obj;
        if (x != other.getX(1))
            return false;
        if (y != other.getY(1))
            return false;
        return true;
    }

We changed our toString() method to access x and y directly. And that’s all there is. Here is the new Hit class in its entirety:

public class Hit {
    double x;
    double y;
    private double r;
    private double theta;

    public Hit(double x, double y) {
        this.x = x;
        this.y = y;
    }

    @Override
    public int hashCode() {
        final int PRIME = 31;
        int result = 1;
        result = PRIME * result + (int)x;
        result = PRIME * result + (int)y;
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        final Hit other = (Hit) obj;
        if (x != other.getX(1))
            return false;
        if (y != other.getY(1))
            return false;
        return true;
    }

    @Override
    public String toString() {
        return "Hit(" + x + "," + y + ")";
    }

    public double r() {
        r = Math.sqrt(x*x+y*y);
        return r;
    }

    public double theta() {
        theta = Math.atan2(y, x);
        if (theta < 0) theta += 2*Math.PI;
        return theta;
    }

    public double getX(double xPixelsPerInch) {
        return x / xPixelsPerInch;
    }

    public double getY(double yPixelsPerInch) {
        return y / yPixelsPerInch;
    }

Where Are We Now?

As I mentioned, we were green every ten minutes or so, so we have confidence that our changes all worked. We didn’t have an overall plan: we were just following our noses, making small changes, then making the necessary other changes to make the program compile. When the program compiled, the tests always went green immediately.

Oddly, though, we didn’t have much of a sense of overall purpose. We were just cleaning up things that we noticed. And yet, as I hope you can see, the code is now a bit better, both in detail, and, we think, architecturally. There is less information shared between the PatterningSheet and the ShotPattern; the member variables of most classes changed are simpler; there are fewer methods than before.

We began with a simple need, to create a density calculation that worked for our customer’s purposes. We built that, making the design a bit worse. Then we smoothed over the code a few times, and now the design is better than it was before we started.

Furthermore, we aren’t even finished. We have a spike version of the density calculation still in the system, we have multiple incompatible constructors in ShotPattern, and there are other infelicities. The tests, however, are all green. We’ll address those further issues in our next meeting, which will be Wednesday, not tomorrow. I’m not sure yet whether I’ll do a little work in the meantime or not. Depends on the weather and what else presents itself.

Bottom Line ...

… or at least interim subtotal …

We have again had the result it seems that we generally have. We have been able to make a substantial architectural change without breaking the system, and without making the overall design worse. It was a two-pass approach: first put in the new capability, which does make the design worse, and then smooth the code, making simple local-feeling changes, until the code is better.

We believe that it’s always possible to do this, not just in smaller examples like this one, but in much larger situations. Of course there is no possible proof for “always” that we can imagine, but every time we set out to do it, we seem to succeed. That’s worth a lot to us, and we hope it’s worth a bit to you.

As always, we look forward to hearing from you. If you write to me, remember to include the string “[ron]” as part of the subject. Thanks!