Shooting for Cleaner Still
The Old Mail Bag
One of the best things about this gig is all the mail we get from people who are interested in what we’re doing, most of it expressing some level of appreciation, and almost all of it offering good ideas. Way cool: keep those cards and letters coming!
Carl Manaster writes:
To add the parameter to getX(), here's what I would have done:
- select getX;
- Refactor > Change Method Signature;
- Add parameter with default value 1.
Author Reply:
That’s cool: we didn’t think of that, and probably wouldn’t have known how to do it if we had thought of it. I can’t remember the last time I used a default parameter value, but this is certainly a perfect use for one.
I do think we’ll find that the code goes somewhere else entirely, getting rid of that parameter. I also think we’ll see that we could have done what’s about to happen first. More on that if and when it happens.
Carl also sent a couple of very nice alternate implementations of our clumping process. I’ve not figured out a good way to publish and describe that. Maybe Carl should do a guest article?
Torbjörn Gyllebring writes again:
After reading the last installment here are some quick thoughts. Isn't the Hit model starting to diverge substantially from itself? By adding the scaling factors to the get{X|Y} functions you've mostly painted a very glum future for r and theta haven't you, the methods only makes sense if you're using a scaling factor of {1,1} or 1:1 that does smell like either abstraction leakage or is telling me that you've prematurely put those methods into the Hit class maybe something to be wary of? Or maybe this is telling us something about missing tests? I'm really interested on hearing your take on it.
Author Reply:
I’m interested in seeing what happens as well. Chet and I spoke about the fact that asking for a scaled value and sending in the scale factor seems odd, and that what we really want is for the Hit to be inherently scaled. We chose to just work over the code using simple steps, accepting that it would probably require several passes to get to where we want. We also observed that the ShotPattern presently has three constructors (if I recall correctly) and they don’t all work the same way with respect to scaling. That needs to be improved.
Adrian Howard offers an idea on the XP list:
Something I've done a couple of times in similar situations was, rather than use random names, use a suffix of "kind_of_" (or similar) so the broken API became something like:kind_of_set_total, kind_of_run_simulation, etc.So the names still made sense, but they were still cues to the whole team that the methods didn't really do what they said they did.
Author Reply:
This is an interesting approach. We weren’t sure whether any of our methods would go away or not, and we were able to come up with a good name in this case, but I like the idea of putting in an obviously temporary name as a hint that something needs to be done.
Local / Global; Design / Code
Dion Stewart, on the leandevelopment list, has been engaging me and others in a discussion of overall design. Frequent readers know that I believe that essentially local refactoring actions, sweeping over the code, can effect global improvements in the code’s design. Dion favors drawing pictures on a white board – and so do I, or the equivalent in a coffee shop where we don’t have a white board.
At the same time, I’ve found that an incredible amount of design improvement can come from observing just a few code smells and improving the scent of the program, in ways that are quite limited in scope and that are essentially local in nature.
It remains an open question how far this can go. It’s hard to imagine changing some inherent architectural feature of some giant program by simple refactoring. At the same time, if we build our giant program in an incremental and iterative fashion, with a coherent team, the architecture will evolve, and it seems fair to imagine that many, perhaps most, of the architectural decisions could evolve into being while the program is still small enough to respond to refactoring of this kind.
What I find in looking at the programs people actually write – and I am privileged to look at many of them in my consulting work – if privileged is the word I was looking for – one doesn’t usually need to understand the whole architecture of the program to do effective work implementing some feature. I would think that part of what “good architecture” means is one where you don’t have to understand the whole program in order to accomplish your goals.
Could it be that in a good program, looked at the right way, most of the decisions one has to make are local? And if so, what does that tell us about overall design and architecture to make that happen? I honestly don’t know. For now, I’m fascinated and delighted at how much design one can do without sweeping changes, and in the concrete presence of real code.
Help and Learning
I’ve commented in the past that I feel that Chet and I get really good help from people when we ask for it, and even when we don’t. Most people probably don’t get help when they don’t ask, and frankly I’ve seen a lot of code where the author should have asked. On reasonably well-chosen lists, you can get rather good help. I have found that people are awfully tolerant of my often terribly ignorant questions about whatever language I’m currently working in for the first time in ages. I hope they’d be that helpful to anyone, not just someone as obviously confused as I am …
Again, thanks to everyone who comments. It helps us, and it helps our readers. It even helps the person who comments, because we always learn when we take the time to think about something. Good all around!
A Solo Flight
Chet can’t come out to play today, and I’m feeling that I’d like to work a little more on the code. The good news is that I’ll probably get more detail into this article. That may also be the bad news, of course, and there’s also the really good chance that I’ll mess something up, perhaps badly.
On the other hand, that’s why we have Subversion running on a server at Chet’s house, and why I’ll go somewhere where there’s Internet, if I get tired of working at home. Either way, my plan is to review the code we’ve been working on, and gently improve it until it submits to my iron will. Let’s review, and then I’m going to relocate to somewhere where I can get an iced Chai Latte.
The interesting bits of Hit class look like this:
public class Hit {
private double x;
private double y;
private double r;
private double theta;
public Hit(double x, double y) {
this.x = x;
this.y = y;
}
public double getX(double xPixelsPerInch) {
return x / xPixelsPerInch;
}
public double getY(double yPixelsPerInch) {
return y / yPixelsPerInch;
}
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;
}
Torbjörn rightly observes that we could be in trouble with our R and Theta stuff. Chet and I rightly observe that all the tests are running, so we aren’t in trouble yet. Torbjörn also inquired whether our tests might not be robust enough. Surely they aren’t, but based on our experience with rectangular density, we’re planning to re-implement the wedge stuff anyway. We might not need to, but we’re assuming we will. In any case, Torbjörn has a good point.
Also possibly worth noting is that the r and theta variables are useless at present, because they aren’t cached. I think they might be there because at one point I wanted to look at them. We should either cache the values or get rid of the variables.
The Hit object is created in just one interesting place, in PatterningSheet:
public class PatterningSheet {
...
public List<Hit> getHits() {
List<Hit> hits = new ArrayList<Hit>();
int width = widthInPixels();
int height = heightInPixels();
int yOffset = height / 2;
int xOffset = width / 2;
for (int y = 0; y < height; y++) {
for (int x = 0; x < width; x++) {
if ((farian.getPixel(x, y, pixelArray)[0]) == 0)
hits.add(makeHit(yOffset, xOffset, y, x));
}
}
return hits;
}
private Hit makeHit(int yOffset, int xOffset, int y, int x) {
Hit hit = new Hit(x,y);
additionalPointsInThisHit(x,y);
return hit;
}
...
In this code we are no longer using the offsets, which should be removed. Chet and I remarked on this yesterday, but owing to the necessity to have lunch, stopped before we did this. On a green bar.
Now the PatterningSheet doesn’t use the Hit for anything, so it should be just dandy if we create it right here with appropriate scale, to the pleasure of the caller of getHits(). That method is called directly right now, in one of the many constructors of ShotPattern:
public class ShotPattern {
int widthInPixels;
int heightInPixels;
List<Hit> hits = new ArrayList<Hit>();
private double widthInInches;
private double heightInInches;
private double xPixelsPerInch;
private double yPixelsPerInch;
public ShotPattern(int width, int height) {
this.heightInPixels = height;
this.widthInPixels = width;
}
public ShotPattern(String fileName) {
this.widthInPixels = 2048;
this.heightInPixels = 1536;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
}
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
widthInPixels = sheet.widthInPixels();
heightInPixels = sheet.heightInPixels();
initPixelsPerInch();
}
private void initPixelsPerInch() {
xPixelsPerInch = widthInPixels / widthInInches;
yPixelsPerInch = heightInPixels / heightInInches;
}
Well, OK, two. That’s why we look at the code. Also, as mentioned, these constructors need to be cleaned up. I would also think that since the whole point of the PatterningSheet is to produce the Hits for the ShotPattern, that we should be doing this all with a single call, perhaps a static method on PatterningSheet class.
The first constructor should concern us. We wonder who is using that, and why. We also find two very scary methods in ShotPattern:
public void addHit(Hit hit) {
hits.add(hit);
}
public void addHit(int x, int y) {
addHit(new Hit(x,y));
}
Someone from the outside can fiddle with our Hits! Who’s doing this? Some tests, of course. I’ll verify that right now: Yes, CenterOfMassTest sends some hits into a ShotPattern, to get hit collections that we could hand-calculate. In addition, DensityTest calls one of the addHit methods as well. This is a bit invasive, but I see the point of wanting to test these functions directly. Of course our DensityTest may be redundant now, testing code that we’re no longer using. Let’s review both those test classes. They’re rather long but straightforward. You’ll want to just scan them and skip on down. The bottom line is that they’re creating ShotPatterns with known configurations of Hits, without going through the trouble of hand-crafting BMP files.
public class CenterOfMassTest {
@Before
public void setUp() throws Exception {
}
@Test
public void trivial() {
ShotPattern shotPattern = new ShotPattern(3,3);
assertEquals(new Hit(0,0), shotPattern.centerOfMass());
}
@Test
public void x4Y9() {
ShotPattern shotPattern = new ShotPattern(10,10);
shotPattern.addHit(new Hit(4,9));
assertEquals(new Hit(4,9), shotPattern.centerOfMass());
}
@Test
public void centerOfMore() {
ShotPattern shotPattern = new ShotPattern(10,10);
shotPattern.addHit(new Hit(-3,+2));
shotPattern.addHit(new Hit(0, +2));
shotPattern.addHit(new Hit(+3, +2));
shotPattern.addHit(new Hit(-3,0));
shotPattern.addHit(new Hit(0,0));
shotPattern.addHit(new Hit(+3,0));
shotPattern.addHit(new Hit(-3,-2));
shotPattern.addHit(new Hit(0,-2));
shotPattern.addHit(new Hit(+3,-2));
assertEquals(new Hit(0,0), shotPattern.centerOfMass());
}
@Test
public void leftOfCenter() {
ShotPattern shotPattern = new ShotPattern(10,10);
shotPattern.addHit(new Hit(-6,+2));
shotPattern.addHit(new Hit(0, +2));
shotPattern.addHit(new Hit(+3, +2));
shotPattern.addHit(new Hit(-6,0));
shotPattern.addHit(new Hit(0,0));
shotPattern.addHit(new Hit(+3,0));
shotPattern.addHit(new Hit(-6,-2));
shotPattern.addHit(new Hit(0,-2));
shotPattern.addHit(new Hit(+3,-2));
assertEquals(new Hit(-1,0), shotPattern.centerOfMass());
}
}
public class DensityTest {
ShotPattern pattern;
@Before
public void setUp() throws Exception {
pattern = new ShotPattern(6,6);
}
@Test
public void fourQuadrants() {
pattern.addHit(1,1);
pattern.addHit(3,1);
pattern.addHit(4,1);
pattern.addHit(0,3);
pattern.addHit(1,3);
pattern.addHit(3,3);
pattern.addHit(4,3);
pattern.addHit(0,4);
pattern.addHit(1,4);
pattern.addHit(3,4);
int[] densities = pattern.analyzeDensity(3,3);
assertEquals(1, densities[0]);
assertEquals(2, densities[1]);
assertEquals(4, densities[2]);
assertEquals(3, densities[3]);
}
@Test
public void wedge0() throws Exception {
Wedge wedge = new Wedge(0, 30, 0, Math.PI/4);
assertEquals(0, wedge.count());
wedge.tally(new Hit(20,5));
assertEquals(1, wedge.count());
wedge.tally(new Hit(25,5));
assertEquals(2, wedge.count());
wedge.tally(new Hit(30,10));
assertEquals(2, wedge.count());
}
@Test
public void wedge4() throws Exception {
Wedge wedge = new Wedge(0, 30, Math.PI, 5*Math.PI/4);
assertEquals(0, wedge.count());
wedge.tally(new Hit(-20,-5));
assertEquals(1, wedge.count());
}
@Test
public void polarDensity() throws Exception {
pattern.addHit(20,5); // Octant 0
pattern.addHit(25,5);
pattern.addHit(10,10); // 1
pattern.addHit(10,20);
pattern.addHit(-5,20); // 2
pattern.addHit(-10,5); // 3
pattern.addHit(-20,10);
pattern.addHit(-20,-5); // 4
pattern.addHit(-15,-10);
pattern.addHit(-20,-10);
pattern.addHit(-5,-20); // 5
pattern.addHit(-5,-25);
pattern.addHit(10,-20); // 6
pattern.addHit(20,-10); // 7
pattern.addHit(50,10); // 8
pattern.addHit(50,20);
pattern.addHit(30,10);
pattern.addHit(30,40); // 9
pattern.addHit(20,40);
pattern.addHit(10,50);
pattern.addHit(-10,50); // 10
pattern.addHit(-10,40);
pattern.addHit(-20,50);
pattern.addHit(-20,40);
pattern.addHit(-40,30); // 11
pattern.addHit(-50,20);
pattern.addHit(-40,10);
pattern.addHit(-50,-10); // 12
pattern.addHit(-40,-20);
pattern.addHit(-40,-30);
pattern.addHit(-30,-40); // 13
pattern.addHit(-20,-40);
pattern.addHit(-20,-50);
pattern.addHit(-10,-50);
pattern.addHit(10,-40); // 14
pattern.addHit(10,-50);
pattern.addHit(20,-40);
pattern.addHit(20,-50);
pattern.addHit(30,-50);
pattern.addHit(50,-10); // 15
pattern.addHit(40,-20);
pattern.addHit(40,-30);
int[] densities = pattern.analyzePolar();
assertEquals(2, densities[0]);
assertEquals(2, densities[1]);
assertEquals(1, densities[2]);
assertEquals(2, densities[3]);
assertEquals(3, densities[4]);
assertEquals(2, densities[5]);
assertEquals(1, densities[6]);
assertEquals(1, densities[7]);
assertEquals(3, densities[8]);
assertEquals(3, densities[9]);
assertEquals(4, densities[10]);
assertEquals(3, densities[11]);
assertEquals(3, densities[12]);
assertEquals(4, densities[13]);
assertEquals(5, densities[14]);
assertEquals(3, densities[15]);
}
}
OK, code review over. I’m going to get suited up and head for Stonehouse Coffee. Take a break, smoke ‘em if ya got ‘em. I’ll be right back.
Ready to Roll
I’m ensconced at Stonehouse, waiting for the line to die down so I can order my drink. I’m starting to think that’s not going to work out. Anyway, we’re ready to code. But let’s think first.
- We've got two ways of getting Hits into the ShotPattern, either as a return from getHits(), sent to a PatterningSheet, or via the addHit() methods. This is one way too many, or two, depending on how you view the two addHit() methods, one of which is a convenience method.
- We would like for the Hits to be scaled to the scale of the ShotPattern, whatever that is.
- The ShotPattern isn't always scaled, because only one of its constructors sets up scale. It seems to me that it should always be scaled, if only 1:1.
- The ShotPattern has three constructors, which don't even call each other.
- The ShotPattern knows the class PatterningSheet, and creates a Sheet directly, just to get the Hits out of it.
- The PatterningSheet knows the Hit class, using it to create a list of Hits to return.
All of these issues are worth thinking about, and they all impact the global design of the system. We certainly wouldn’t have to look far to find someone who would tell us that our model classes shouldn’t know the names of other classes, and to suggest that we have an opportunity to use “Dependency Injection” to eliminate that “problem”. We could find advisors who would suggest that the relationship of the ShotPattern to the patterning sheet could be expressed as an interface, perhaps named something like HitProvider, with just one method in it, getHits().
If we had that, then we could use the “Self Shunt” pattern to make our tests implement getHits() and pass themselves to the ShotPattern as the HitProvider.
Alternately, we could make the addHit() method the official way of providing hits to a ShotPattern, and make that the place where scaling takes place as well. Presumably we’d do that with an interface as well, perhaps HitConsumer.
Presently, we’re scaling the Hits at the time we retrieve their X and Y values. That’s clearly bad. They should either be created as scaled, or should be scaled once and for all. (This is one point where Torbjörn’s notion of a World Transform comes to mind.)
As things are written, everyone, and it does seem like everyone, is passing a scale factor to the Hits. Not a good thing, that. Most of the users don’t know and don’t care, and are using 1 as a scale factor. The only calls that use non-unit scaling are in our new countHits() method in ShotPattern itself:
public int countHits(Rectangle2D rectangle) {
int count = 0;
for (Hit hit: hits) {
if (rectangle.contains(hit.getX(xPixelsPerInch), hit.getY(yPixelsPerInch)))
count++;
}
return count;
}
There’s also a minor anomaly, in that the CenterOfMass method returns an instance of Hit. That’s just a convenience: it could have been a point, or any other object containing an X and a Y.
There’s a lot to think about, and Chet and I generally think and talk about all these things. Because we’re talking, all the considerations don’t always get put into the articles – let us know which way you prefer.
What to Do, Where to Start?
Chet and I were taught by Kent Beck to favor constructing objects once and for all, rather than building them part way and then sending them messages until they’re all set up. So I am not inclined, and I bet Chet will agree, not to bless the addHit() interface on ShotPattern.
This line of thinking makes me believe that ShotPattern should be created directly with what it needs to know, including a collection of Hits. Let’s review those constructors:
public ShotPattern(int width, int height) {
this.heightInPixels = height;
this.widthInPixels = width;
}
public ShotPattern(String fileName) {
this.widthInPixels = 2048;
this.heightInPixels = 1536;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
}
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
widthInPixels = sheet.widthInPixels();
heightInPixels = sheet.heightInPixels();
initPixelsPerInch();
}
private void initPixelsPerInch() {
xPixelsPerInch = widthInPixels / widthInInches;
yPixelsPerInch = heightInPixels / heightInInches;
}
Further exploration tells us that although the second constructor assumes 2048x1536, it’s used with some BMP files that aren’t really of that dimension. The only uses of those values are in setting up the pixels per inch, and in the analyzeDensity method, which is on the verge of obsolete. There are only two tests of analyzeDensity, and since density has been redone, and is well tested on its own, I think I’ll start by removing that method.
At Last, Down to Work
It may seem like a large amount of design has just taken place, but normally it would all be done in a short conversation. Mind you, we’re not at all opposed to some design conversations: we just want to turn them to code as soon as possible. So here goes, removing analyzeDensity, which used to look like this, in ShotPattern:
public int[] analyzeDensity(int width, int height) {
int[] result;
int numX = this.widthInPixels / width;
int numY = this.heightInPixels / height;
result = new int[numX*numY];
Rectangle rect = new Rectangle(width-1, height-1);
int location = 0;
for (int top = 0; top < this.heightInPixels; top+= height) {
for (int left = 0; left < this.widthInPixels; left+=width) {
rect.setLocation(left, top);
result[location] = density(rect);
location++;
}
}
return result;
}
Rip. Out it goes, and its little dog density() too. I remove the associated tests, and we’re all green. I’ll check the FitNesse tests as well. They’re all good, including the density test.
All our density calculations now go through the new RectangularGrid object, which is well-tested on its own, in the FitNesse test and in RectangularDensityTest. And DensityTest still tests the Wedge and the radial density spikes. I’ll preserve those for now.
Now ShotPattern only uses widthInPixels and heightInPixels in the scaling initialization. And quite often, those values are completely notional, as, of course, are the width and height in inches variables as well. For now, let’s fold the constructors together.
No, wait. Just for fun, I removed the assignment of the width and height in pixels from the first constructor, which only takes height and width. The tests all still run. That makes me think I can change the constructor itself to have no parameters. I did that, and the tests still run. Now I’ll look for senders of that version: I assume that they’ll all just be passing in their own Hits. Sure enough, that’s what’s happening. They’re just testing things like CenterOfMass, which just process the Hit collection on ShotPattern, without regard to anything else.
So we should be able to get rid of the member variables for height and width in pixels. The second constructor sets them, but need not. So I remove those lines, resulting in this:
public ShotPattern() {
}
public ShotPattern(String fileName) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
}
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
widthInPixels = sheet.widthInPixels();
heightInPixels = sheet.heightInPixels();
initPixelsPerInch();
}
private void initPixelsPerInch() {
xPixelsPerInch = widthInPixels / widthInInches;
yPixelsPerInch = heightInPixels / heightInInches;
}
Now let’s inline the init, which will let us make the width and height in pixels local. After inlining:
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
widthInPixels = sheet.widthInPixels();
heightInPixels = sheet.heightInPixels();
xPixelsPerInch = widthInPixels / widthInInches;
yPixelsPerInch = heightInPixels / heightInInches;
}
And then making them local:
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
int widthInPixels = sheet.widthInPixels();
int heightInPixels = sheet.heightInPixels();
xPixelsPerInch = widthInPixels / widthInInches;
yPixelsPerInch = heightInPixels / heightInInches;
}
Then inlining:
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
this.widthInInches = widthInInches;
this.heightInInches = heightInInches;
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
xPixelsPerInch = sheet.widthInPixels() / widthInInches;
yPixelsPerInch = sheet.heightInPixels() / heightInInches;
}
We notice that widthInInches and heightInInches are used nowhere else, so there’s no reason for the member variables or the assignments:
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
xPixelsPerInch = sheet.widthInPixels() / widthInInches;
yPixelsPerInch = sheet.heightInPixels() / heightInInches;
}
Now the only user of the pixelsPerInch is the countHits() method:
public int countHits(Rectangle2D rectangle) {
int count = 0;
for (Hit hit: hits) {
if (rectangle.contains(hit.getX(xPixelsPerInch), hit.getY(yPixelsPerInch)))
count++;
}
return count;
}
If the hits were scaled all the time, this method would be simplified and the pixels per inch variables wouldn’t be needed. Furthermore, the way things are now, if countHits() is called when we’ve been created with the first two constructors, bad things will happen anyway. Let’s convert the hits right after creating them:
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
xPixelsPerInch = sheet.widthInPixels() / widthInInches;
yPixelsPerInch = sheet.heightInPixels() / heightInInches;
convertHits(xPixelsPerInch, yPixelsPerInch);
}
private void convertHits(double xPixelsPerInch, double yPixelsPerInch) {
for ( Hit hit: hits)
hit.scale(xPixelsPerInch, yPixelsPerInch);
}
This begs for scale() on Hit, and an adjustment to the get methods:
public void scale(double xPixelsPerInch, double yPixelsPerInch) { x = x / xPixelsPerInch; y = y / yPixelsPerInch; } public double getX(double xPixelsPerInch) { return x; } public double getY(double yPixelsPerInch) { return y; }
Tests are green, by the way, and have been right along. Now we can change the signature on getX and getY:
public double getX() { return x; } public double getY() { return y; }
Now, if you’ll forgive me, I intend to rename those methods to x and y, because our local coding standard mostly hates methods named getSomething. If we have others, we’ll go after them later. That leaves Hit looking like this:
public class Hit {
private double x;
private double y;
private double r;
private double theta;
public Hit(double x, double y) {
this.x = x;
this.y = y;
}
public void scale(double xPixelsPerInch, double yPixelsPerInch) {
x = x / xPixelsPerInch;
y = y / yPixelsPerInch;
}
public double x() {
return x;
}
public double y() {
return 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;
}
@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.x())
return false;
if (y != other.y())
return false;
return true;
}
@Override
public String toString() {
return "Hit(" + x + "," + y + ")";
}
}
Not bad. But I’m still on a roll and have a few minutes before lunch time. Let’s look back at those ShotPattern constructors:
public ShotPattern() {
}
public ShotPattern(String fileName) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
}
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
xPixelsPerInch = sheet.widthInPixels() / widthInInches;
yPixelsPerInch = sheet.heightInPixels() / heightInInches;
convertHits(xPixelsPerInch, yPixelsPerInch);
}
We can try folding the second into the first this way:
public ShotPattern(String fileName) {
this(fileName, 1, 1);
}
That doesn’t work: the callers of this method expect that their pixels per inch are 1. So instead, I change the callers:
@Test
public void useRasterToCreateShotPatternBiggerFile() {
ShotPattern shotPattern = new ShotPattern(folder + "PB270011.bmp", 2048, 1536);
assertEquals(new Hit(1139,470), shotPattern.centerOfMass());
}
@Test
public void centerOfSmallPicture() {
ShotPattern shotPattern = new ShotPattern(folder + "x4y9on7x12.bmp", 7, 12);
assertEquals(new Hit(4,9), shotPattern.centerOfMass());
}
I also have to change a couple of other uses of the single-parameter method, in the FitNesse tests, and in the CreatePatternImage main program. I’ll spare you the latter. Here’s the FitNesse:
public class CenterOfPatternFixture extends ColumnFixture {
final String folder = "..\\Data\\";
public String inputFileName;
public int x(){
ShotPattern pattern = new ShotPattern(folder + inputFileName, 2048, 1536);
return pattern.centerOfMassXinches();
}
public int y(){
ShotPattern pattern = new ShotPattern(folder + inputFileName, 2048, 1536);
return pattern.centerOfMassYinches();
}
}
This isn’t quite right. This fixture should be extended to allow other than 2048x1536 files, but it’s good enough for now.
Wrong! It’s not good enough for now. We use the FitNesse fixture CreatePatternFromFile in two places, and one of them uses a different file size.
The real fix, of course, is to get the file size from the file, and have a way of opening a file and scaling one to one. A quicker fix would be to add x and y fields to the FitNesse tests.
The best thing for now, however, since time is up, is to put our single-parameter method back, and leave folding the three constructors together for another day. This is particularly true in view of our observation that the ShotPattern knows too much about the PatterningSheet anyway. So I’ll leave the tests that are converted to the three-parm version as is, and change the other two back, and provide them with their constructor …
No, that doesn’t fix the FitNesse test. My unit tests are green, and one FitNesse test, FileReadTest, doesn’t run. I can’t ship this code, but I had better check it in. We’ll pick up later …
After Lunch
I had a sumptuous repast at Wendy’s, where I also spent a little time relaxing and reading a fiction book. Now I’d like to take a look at that one broken FitNesse test. Here’s the bad news:
I’m sure that the issue will be a simple one having to do with the scaling, since that’s really all that we have changed. I could fix it directly, I imagine, but since the unit tests didn’t show up this problem, it would be wise to try to duplicate the issue that way. Let’s review the fixtures shown in the test, CreatePatternFromFile, and CenterOfMass:
public class CreatePatternFromFile extends ColumnFixture {
public String fileName;
final String folder = "..\\Data\\";
public static ShotPattern pattern;
public Boolean doIt(){
pattern = new ShotPattern(folder + fileName, 4.0, 3.0);
return true;
}
public class CenterOfMass extends ColumnFixture {
public int xCenter() {
return (int) CreatePatternFromFile.pattern.centerOfMass().x();
}
public int yCenter() {
return (int) CreatePatternFromFile.pattern.centerOfMass().y();
}
}
Well, I see the problem, I think. It’s that 4.0, 3.0, which is the appropriate dimension for our other use of this fixture, but not this one. I’m not sure, however, why it just started failing.
The BMP file in question in this test is 7x12 pixels in size. This makes me wonder what would happen if we put 7 and 12 into the test. Would it start to work? I can’t resist finding out. Yes. As expected, it works. Of course, the other test that uses this fixture breaks.
We have changed the FitNesse fixture to use the version of the setup that sets the picture size, and our two pictures are two different sizes. The easiest fix I can see is to add width and height to the CreatePatternFromFile fixture. This won’t solve my concern about not having a broken unit test, however. For now, I think I’ll go ahead and change the FitNesse tests and fixture:
public class CreatePatternFromFile extends ColumnFixture {
public String fileName;
public int columns;
public int rows;
final String folder = "..\\Data\\";
public static ShotPattern pattern;
public Boolean doIt(){
pattern = new ShotPattern(folder + fileName, columns, rows);
return true;
}
}
The unit tests are also green. We’re all good, except that I didn’t follow the ritual of writing a failing unit test to show the defect. The problem is, the defect was that the fixtures were calling the object’s constructor with the wrong parameters. I don’t feel that I need much of a test to show that wrong parameters give wrong results.
You might disagree, so write if you can think of what I should test. And I’ll take it up with Chet tomorrow when we get together in Ann Arbor. Meanwhile, everything is green. I’m checking in.
Retrospective
This article has much more detail of individual steps than most of the preceding ones in this series. Let us know whether you think that’s good, or not so good, and we’ll see what we can do to accomodate your preferences.
The steps taken were all about the same size as our steps in previous articles, it’s just that they were written down in more step by step detail. Things went pretty smoothly, except that the FitNesse test breaking caught me by surprise. I don’t know what to do there, other than run them more often. Advise me.
As far as the code goes, the Hit class is simpler now:
public class Hit {
private double x;
private double y;
private double r;
private double theta;
public Hit(double x, double y) {
this.x = x;
this.y = y;
}
public void scale(double xPixelsPerInch, double yPixelsPerInch) {
x = x / xPixelsPerInch;
y = y / yPixelsPerInch;
}
public double x() {
return x;
}
public double y() {
return 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;
}
@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.x())
return false;
if (y != other.y())
return false;
return true;
}
@Override
public String toString() {
return "Hit(" + x + "," + y + ")";
}
}
And so is ShotPattern, though it’s not perfect yet:
public class ShotPattern {
List<Hit> hits = new ArrayList<Hit>();
private double xPixelsPerInch;
private double yPixelsPerInch;
public ShotPattern() {
}
public ShotPattern(String fileName, double widthInInches, double heightInInches) {
PatterningSheet sheet = new PatterningSheet(fileName);
hits = sheet.getHits();
xPixelsPerInch = sheet.widthInPixels() / widthInInches;
yPixelsPerInch = sheet.heightInPixels() / heightInInches;
convertHits(xPixelsPerInch, yPixelsPerInch);
}
private void convertHits(double xPixelsPerInch, double yPixelsPerInch) {
for ( Hit hit: hits)
hit.scale(xPixelsPerInch, yPixelsPerInch);
}
public Hit centerOfMass() {
if (hits.size() == 0) return new Hit(0,0);
return new Hit(xCenter(), yCenter());
}
private int yCenter() {
int sum = 0;
for (Hit hit : hits) {
sum += hit.y();
}
return sum/hits.size();
}
private int xCenter() {
int sum = 0;
for (Hit hit : hits) {
sum += hit.x();
}
return sum/hits.size();
}
public void addHit(Hit hit) {
hits.add(hit);
}
public void addHit(int x, int y) {
addHit(new Hit(x,y));
}
public int[] analyzePolar() {
Wedge[] wedges = createWedges();
for (Wedge wedge: wedges) {
for ( Hit hit: hits) {
wedge.tally(hit);
}
}
int tally = 0;
int[] result = new int[16];
for (Wedge wedge: wedges) {
result[tally++] = wedge.count();
}
return result;
}
private Wedge[] createWedges() {
Wedge[] wedges = new Wedge[16];
int wedge = 0;
int deltaR = 30;
double deltaTheta = Math.PI/4;
for (int radius = 0; radius < 60; radius += deltaR) {
for (double theta = 0; theta < 2*Math.PI; theta += deltaTheta) {
wedges[wedge++] = new Wedge(radius, radius+deltaR, theta, theta+deltaTheta);
}
}
return wedges;
}
public int centerOfMassXinches() {
double rawX = centerOfMass().x();
return (int) Math.round(rawX / 51.2);
}
public int centerOfMassYinches() {
double rawY = centerOfMass().y();
return (int) Math.round(rawY / 38.4);
}
public int countHits(Rectangle2D rectangle) {
int count = 0;
for (Hit hit: hits) {
if (rectangle.contains(hit.x(), hit.y()))
count++;
}
return count;
}
}
There are some odd methods left in there, including the centerOfMass ones that separate X and Y, and as I mentioned before, I expect that the analyzePolar may disappear or change radically once we get working on the story about that picture. One thing that will definitely change is that Chet wants the sectors arranged around the center of mass, not the target center point.
We still haven’t fully separated the ShotPattern and what it knows from the PatterningSheet and what it knows, but we are very close. The perenniel Kelly Anderson offered a thought on the testdrivendevelopment list that may be useful. He suggested the use of a unit square, from 0,0 to 1,1, for representation of the hits. Now, in ShotPattern, I still think we need user coordinates, presently thought of as inches, though they really have nothing to do with real inches.
However, in PatterningSheet, creating the hits in the unit square would mean that ShotPattern doesn’t need to know how many pixels there are in the actual picture. It would only need to know how many nominal units it wanted to think in terms of, and the scaling would always be from the unit square to the desired units. That will reduce coupling between those two classes, and move us along the way to the PatterningSheet being little more than an I/O tool for creating hit lists. That seems like a good thing.
Going forward, I’d really like to try changing the PatternSheet to report hits in the unit square. The more I think about Kelly’s idea, applied this way, the better I like it.
It’s also worth thinking about Torbjörn’s notion of a World Transform. Perhaps instead of explicitly transforming the Hits, we should do that whole thing another way, with a built-in transform of some kind. I can think of some really neat ways to do that: are any of them justified at this point? I suspect not. We have the capabilities isolated where we want them, and that’s probably good enough for now.
I’d like to get rid of the odd ShotPattern creator, and the addHit() methods. One way to do that would be to extract an interface from PatterningSheet and overload it with a test-focused implementation. Another way, perhaps just as good, would be to create the Hit list and create the ShotPattern on it, instead of having it create the PatterningSheet in its constructor. A third, intermediate, way might be some factory methods on the ShotPattern class. None of these seem terribly important right now.
At the end of the day, we have cleaner code, and a cleaner design. Tomorrow we’ll decide whether to go further with cleanup, or to work on a customer story. I’m inclined to the latter.
Lessons?
As Torbjörn suggested, we’re getting a little scent of weakness in our tests. I’m confident that the code is working, but the tests are not covering it as strongly as they might. In particular, I’m disappointed that a FitNesse test failed without a JUnit test failing first. I like for the FitNesse tests to be a second line of defense, and for them rarely to be needed. This one was needed … and we had it, which was good, even if all it detected was a problem in the fixture itself. Still, it reduces my confidence a bit. Must pay attention to those things.
The path we took to get here was a bit longer than we could have taken, at least in retrospect. Of course I’d rather take the shortest path – in fact I’d prefer to get everything right the first time. That doesn’t seem to happen all that often, so I’ve come to be quite comfortable with the knowledge that sanding the code smoother in small steps works quite nicely.
I felt that I got to tell you more about what I was doing today, but I didn’t feel as steady as I would have had Chet and I been working together. I do think that I program rather competently and that I can in principle do anything necessary All By Myself. In practice, things go better with two.
Again, I did not check my code into Subversion until I was down to that point with the failing FitNesse test. I’d have been better off, arguably, to have reverted a green bar or so back. Having worked without Subversion for so long, I don’t have a frequent check-in habit. I’ll try to figure out how to get one going.
To me, the main lesson is one that appears again and again. It’s straightforward to identify design problems by looking at the code, and straightforward to smooth them out bit by bit, incrementally. It would be “better” to get everything right up front. Maybe you’re good enough to do that every time: I know I’m not. I’d offer the notion that skill in this kind of design improvement by refactoring will pay off in those frequent moments when things don’t work out quite as we planned. I know it does for me.
Thanks for tuning in. See you next time, same bat time, same bat channel.