protected Bid prepareBid() {
Bid result = null;
if (calculator.getCombinedPoints() < 13) {
result = getValidBidFor6LongSuitOrDecent5(1);
} else if (calculator.getCombinedPoints() < 16) {
result = getValidBidForSuit5AndLonger(1);
if (result == null) {
result = getValidBidFor6LongSuitOrGood5(2);
}
} else if (calculator.getCombinedPoints() < 19) {
result = getValidBidForSuit5AndLonger(1);
if (result == null) {
result = getValidBidForSuit5AndLonger(2);
}
}
return result;
}
Here is what I ended up with 2 hours later:
protected Bid prepareBid() {
if (calculator.getCombinedPoints() < 13) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(6)), //
bidSuit(1, hand.getDecent5LengthSuits()));
} else if (calculator.getCombinedPoints() < 16) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(5)), //
bidSuit(2, hand.getSuitsWithAtLeastCards(6)), //
bidSuit(2, hand.getGood5LengthSuits()));
} else if (calculator.getCombinedPoints() < 19) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(5)), //
bidSuit(2, hand.getSuitsWithAtLeastCards(5)));
}
return null;
}
Was it worth the time?
The first implementation seems pretty straightforward, but one level deeper, looking at the methods getValidBidFor6LongSuitOrDecent5(int x), etc., there was a lot of duplication and murkiness. Duplication of logic and multiple responsibilities can be inferred from method names. Also, I could not make it clear what the parameter passed to the method was (as you can see below, it was bid level). Take a look at the following two methods, and note how murky they are. Method name is the only thing that can explain what is happening, the code itself is doing too much to make it easy to follow.
private Bid getValidBidForSuit5AndLonger(int maxBidLevelAllowed) {
List suits5AndLonger = hand.getSuitsWithAtLeastCards(5);
if (suits5AndLonger.size() > 0 &&
auction.isValid(new Bid(maxBidLevelAllowed, suits5AndLonger.get(0)))) {
return new Bid(maxBidLevelAllowed, suits5AndLonger.get(0));
}
return null;
}
private Bid getValidBidFor6LongSuitOrDecent5(int maxBidLevelAllowed) {
List goodSuits5AndLonger = hand.getSuitsWithAtLeastCards(6);
goodSuits5AndLonger.addAll(hand.getDecent5LengthSuits());
for (Suit suit : goodSuits5AndLonger) {
if (auction.isValid(new Bid(maxBidLevelAllowed, suit))) {
return new Bid(maxBidLevelAllowed, suit);
}
}
return null;
}
The two additional methods not listed, followed the pattern in getValidBidFor6LongSuitOrDecent5(). There's logical duplication in what these methods are doing. It's around placing a valid bid based on a set of suits meeting certain criteria.
This code didn't just get slapped together. Up to this point I was being very meticulous, and have already taken several refactoring passes through it. And yet it was smelling.
I decided to tackle the duplication I noticed, to see whether it'd affect the top level method, prepareBid() and if it'd be overall good or bad for clarity. Whenever I saw Or criterion in method name, I split it into two methods. I then combined the methods by extracting the bid placement logic, and passing the selected suits as parameters to the generic method. The generic method ended up looking a lot like constructor of the Bid class: new Bid(1, CLUBS), which made me think I was on the right path. Take a look:
private Bid bidSuit(int bidLevel, CollectionHowever, the top level method, prepareBid was now looking somewhat messy:suits) { for (Suit suit : suits) { if (auction.isValid(new Bid(bidLevel, suit))) { return new Bid(bidLevel, suit); } } return null; }
protected Bid prepareBid() {
Bid result = null;
if (calculator.getCombinedPoints() < 13) {
result = bidSuit(1, hand.getSuitsWithAtLeastCards(6));
if (result == null) {
result = bidSuit(1, hand.getDecent5LengthSuits());
}
} else if (calculator.getCombinedPoints() < 16) {
result = bidSuit(1, hand.getSuitsWithAtLeastCards(5));
if (result == null) {
result = bidSuit(2, hand.getSuitsWithAtLeastCards(6));
if (result == null) {
result = bidSuit(2, hand.getGood5LengthSuits());
}
}
} else if (calculator.getCombinedPoints() < 19) {
result = bidSuit(1, hand.getSuitsWithAtLeastCards(5));
if (result == null) {
result = bidSuit(2, hand.getSuitsWithAtLeastCards(5));
}
}
return result;
}
The nested ifs are a smell. So are the null checks. In this case they indicate logic around choosing the first possible match, and then moving on to another. The same smell present in the original version of the method, quoted at the beginning of this post, but refactoring exposed it completely. At this point it was not difficult to wrap this logic into a separate method.
And here is, once again the end result:
protected Bid prepareBid() {
if (calculator.getCombinedPoints() < 13) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(6)), //
bidSuit(1, hand.getDecent5LengthSuits()));
} else if (calculator.getCombinedPoints() < 16) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(5)), //
bidSuit(2, hand.getSuitsWithAtLeastCards(6)), //
bidSuit(2, hand.getGood5LengthSuits()));
} else if (calculator.getCombinedPoints() < 19) {
return firstValidBid( //
bidSuit(1, hand.getSuitsWithAtLeastCards(5)), //
bidSuit(2, hand.getSuitsWithAtLeastCards(5)));
}
return null;
}
private Bid bidSuit(int bidLevel, Collection suits) {
for (Suit suit : suits) {
if (auction.isValid(new Bid(bidLevel, suit))) {
return new Bid(bidLevel, suit);
}
}
return null;
}
private Bid firstValidBid(Bid... bids) {
for (Bid bid : bids) {
if (bid != null) {
return bid;
}
}
return null;
}
The top level method is more readable than the version in the beginning of this post and in the process I got rid of nearly 20 lines of duplicated logic. Was it worth the extra 90 minutes that it took? The standard argument brought up is that 90% of code's lifetime is spent in maintenance mode, so we get a large return on effort investment over time. This may seem a bit academic, so let me give a couple of personal practical reasons.
First, I can now go much faster with the next steps. This rule is far from being done, and lumping along 30% duplicated logic as I was implementing more and more cases would make it harder to maintain and more likely that I'd add even more duplicated logic. Having a clean picture right now lets me easier identify when the code gets messy again, and attack it.
Second, I would like to attract more developers to the project. If a researcher wants to popularize his work, he better invest the time to prepare a good seminar to present that work. The first version of the code was pretty solid, and was passing all the tests. But it is somewhat equivalent to taking lab notes to a seminar and improvising in front of the audience, in hopes of conveying the idea. I have tried explaining complicated problems to a lecture hall of 100+ students, and it takes a lot of preparation to even get to the point where just a fraction of the audience can be reached. A good lecture will take a complicated problem, break it down into simple chunks, and convey the material in a language that the audience can understand. While it takes a long time to prepare, it is very rewarding to the speaker and the audience. I think leaving clean code behind is a necessary step in engaging developers with Gnubridge.
[1] For reference, here's domain expert's description of the logic that the above code implements (Pavlicek's Bridge Basics Online, lesson 7):
[...]you may be able to make a suit overcall. This is done by bidding your suit at the cheapest level possible.
A suit overcall at the one level requires 10-18 points (distributional points included). Further, if your strength is toward the minimum range (10-12), you should have a decent five-card suit — at least Q-J-x-x-x — or any six-card suit.
A suit overcall at the two level requires about 13-18 points, and here the suit quality is even more important. If your strength is minimum (13-15), you should have a good five-card suit — at least A-Q-J-x-x or K-Q-10-x-x — or any six-card suit.
Posts
