Wednesday, February 11, 2015

Things that Java got wrong, part 3

Last time in this series (Things that Java got wrong, part 2), I talked about interfaces and the lack of an ability to include default method bodies. Happily, this major oversight has been fixed in Java 1.8.

But now I want to rail against another aspect of interfaces and abstract types (or rather their usage) that I think is by far the worst thing that the Java designers ever messed up. The "Number" type.

First, let's look at three simple reasons why java.lang.Number is so bad.
  • it should be an interface -- or rather several interfaces -- but instead it is an abstract type;
  • if it is going to be an abstract type, then at least let's have it implement Comparable<Number> --- but it doesn't; all of its sub-classes, say X, implement Comparable<X> but that's not the same thing at all: if you need a generic type that implements Number and Comparable, it can't be done without creating your own type!
  • it doesn't even have a method to let you find out if the type is integral or real (forget about complex) -- you have a Number, you can check if it implements Integer, Long, etc. but if somebody creates a new sub-class of Number that happens to be integral, you won't catch it.
I ran into all of these problems recently while working on a new open-source framework for dealing with fuzzy objects (i.e. objects with uncertainty) and they caused me some big headaches. You can find the project at FuzzyJ.

Let's think about how we would go about defining an interface (or interfaces) to represent numbers. Sounds pretty straightforward, right? But it isn't quite that simple. There's a world of difference between the integers, where the successor or predecessor operators make perfect sense, and the real numbers where those operators don't make much sense -- while operators such as round are useful. And then there are complex numbers, rational numbers, irrational numbers, etc. etc. In other words, different types of numbers require different methods. In fact, to put it another way by inverting the question, the operators essentially define the number classes. Is there a fundamental set of operators that would apply to all numbers? There really isn't. But a reasonable set that works with most types of number is this: addition, multiplication, negation, perhaps some others, including compare with.

But already we run into problems. If the set of numbers you're modeling is the positive integers, then negation makes no sense.

So, let's start out with something like this:

public interface Numeric extends Comparable<numeric> {
 Numeric add(Numeric other);
 Numeric multiply(Numeric other);
}
This will work for the positive integers and most other classes. If we want to extend the class to all integers, then we can define the following:
public interface Integral extends Numeric {
 Numeric negate(Numeric other);
}

So far so good. We can now define an IntegralBase class based on the int primitive:

public class IntegralBase implements Integral {

 private int value;

 public IntegralBase(int value) {
  super();
  this.value = value;
 }

 @Override
 public Numeric add(Numeric other) {
  if (other instanceof IntegralBase)
   return new IntegralBase(this.value+((IntegralBase) other).value);
  throw new RuntimeException("cannot add non-IntegralBase object");
 }

 @Override
 public Numeric multiply(Numeric other) {
  if (other instanceof IntegralBase)
   return new IntegralBase(this.value*((IntegralBase) other).value);
  throw new RuntimeException("cannot multiply non-IntegralBase object");
 }

 @Override
 public int compareTo(Numeric other) {
  if (other instanceof IntegralBase)
   return Integer.compare(this.value, ((IntegralBase) other).value);
  throw new RuntimeException("cannot add non-IntegralBase object");
 }

 @Override
 public Integral negate(Integral other) {
  return new IntegralBase(-this.value);
 }
}
But we're already beginning to get into difficulties. We don't have anything good to do if we try to add (multiply, or compare) an object which is not Integral (or, more specifically, an IntegralBase). What if the "int" primitive isn't sufficient for our purposes and we need a BigInteger? We could define a BigIntegral class just like the one above. Or we could make Integral generic, except of course that "int" cannot be a generic type because it's a primitive.

But even this is better than the setup that the Java designers gave us. What we have in Java is an abstract type (not an interface) called Number.

public abstract class Number implements java.io.Serializable {

    public abstract int intValue();

    public abstract double doubleValue();

    // etc. etc.
}
That's basically all there is apart from longValue, floatValue, etc. There's no good way to find out if the object we are dealing with is a whole number (operable with one set of operations) or a real number (operable with another set, with some overlap).

The designers of the math3 package from Apache "commons" have helped somewhat. They do bring in a little mathematics withe the Field and FieldElement interfaces. And they provide a type for rational numbers in BigFraction.

But in my humble opinion, Java, while it is admittedly a general-purpose language, could have done so much better right from the start.

OK, back to work.

Monday, November 17, 2014

TiVo

I love TiVo, that's to say I love digital video recorders. I've been letting TiVo simplify my life -- and avoid commercials -- for 13 years now.

Nevertheless, I'm going to use the TiVo user interface as an example of how not to write user interfaces. It seems that they cobbled together something pretty basic when they got started in 1999 and they haven't improved it since. There have been a few minor tweaks and/or name changes but nothing substantial. I don't have the Roamio -- maybe the user interface there is different [see postscript] -- but the classic UI on my "series 3" is simply a bad design that has never been fixed.

According to Wikipedia, there are seven principles of user interface design. While the TiVo design does an adequate job with six of the seven principles, I believe it falls quite short in the seventh:
  • Conformity with user expectations: the dialogue conforms with user expectations when it is consistent and corresponds to the user characteristics, such as task knowledge, education, experience, and to commonly accepted conventions.
What this says in other words is that the UI should operate on the same model of the world (or, more specifically, the relevant subset of the world) as does the user. That makes it user-centric, rather than information-centric, system-centric or whatever. It is the job of the UI (not the user) to translate between the user's model of the world and the system's internal model.

Let me start with the simplest and most fundamental error: when you are, say, watching live TV and you go up to the top-level menu, you would naturally expect that "live TV" would be the current selection. But no, "Now Playing List" is the new selection. That means that if you inadvertently clicked up to the menu and then pressed "Select" you would expect to be back watching live TV -- but you aren't. That breaks perhaps the #1 rule of user interface design: the principle of least surprise. Or, to put it in terms of the above definition, the UI is supposed to be conform to user expectations and be consistent.

Another major mismatch between the TiVo UI model and the way viewers think: channels. Back in the day when there were just a few channels available, essentially one per network, the concept of a channel meant something. You just "knew" which channel a program would be on and it didn't make any sense for it to be on a different channel. But that situation was long gone, here in the USA at least, when TiVo was introduced so it has never made any sense. The viewer simply doesn't care which channel something is on. And, truth be told, neither does TiVo. Yet the user is required, when setting up a Season Pass, for example, to specify the channel. The Season Pass largely ignores this information because it actually lists all of the upcoming episodes, regardless of channel. The user does distinguish between first-run and repeats. And TiVo asks about that. But when listing episodes, it doesn't make any distinction. Consistency!

Another issue that is a fundamental breach of UI design (but strangely is not mentioned in the Wiki article) is that the controller should always be "live." That is to say, there should never be an operation that the user can initiate that he can't cancel or switch to some other operation. Frequently, TiVo goes into a funk while it is reacting to a user command -- and the user is helpless until the action finishes. And there isn't even an indication of how long the action is likely to take.

But my biggest complaint of all is that TiVo has not changed the model to accommodate high-definition TV. Although HDTV was, in theory at least, around in 1999 when TiVo was launched, it didn't become mainstream until the mid-2000s. The PBS HD channel began operations in 2004, for example. Should TiVo have anticipated HD? Of course they should, but it probably would have been acceptable for them to remodel the UI after their first few years of operation. Note that I am talking about the UI here. At some point (around 2005?) new TiVos did support recording and playing HD programs. But the UI continues in blissful ignorance of this rather important concept. For example, when setting up a Season Pass, you cannot specify that you do (or do not) want to record in HD. You can try to persuade the TiVo by specifying you want to record from an HD-only channel (in order to do this, however, you have to delete the old season pass and reprogram it -- unbelievable). But even then, the only way you can insist that a program be recorded in HD is to tell TiVo that you don't receive the corresponding non-HD channel(s). Bizarre in the extreme.

There are many other issues that I have with the TiVo UI. Things that they certainly ought to have fixed in 13 years! But I've covered the main points.

The conclusion? When you're designing a UI, don't think about the way your system works, or how the information is stored in your internal storage. Think about the way the user will want to interact with the system, how he or she will "think" about what they are doing. Model that instead and make all of the interactions consistent with that model. Yes, that's work. But isn't that what your paid for?

OK, back to work!

Postscript: I drafted this on 10/31 and the very next day my TiVo expired (the fan stopped working). No, I don't think it was a conspiracy between Google and TiVo. The TiVo people were quite helpful in getting me an upgrade to the Roamio. It was a significant operation to get it working, requiring collaboration with three different and not entirely cooperative entities: TiVo, Comcast and me. And my old expander disc, while "compatible" with the new model, is completely unreadable. So, basically, I lost everything that I had previously recorded. This seems the height of poor system design. Why on earth would they consider the internal disc and the external disc to be one single volume?

But the look and feel has improved enormously. The TiVo menus are now in HD and they have fixed quite a few of the problems I mentioned above. How is it, though, that those improvements were not available to the old Series 3? There are still breaches of the principle of least surprise. For instance, if you set up a season pass now and choose "new" only, the default channel chosen will, it seems, most likely be a channel that only shows re-runs. You can change it if you happen to notice, but TiVo will not warn you that the season pass will do nothing.

Wednesday, August 27, 2014

Exception handling -- part 2

I last previously talked about exception handling in a blog a couple of years ago: Exception Handling. That was a fairly short blog which attempted to cure some of the more nefarious problems in exception handling which I sometimes see in code. Following these recommendations (nothing that isn't already very obvious) will result in "OK" code.

Now, I want to write up some more advanced guidelines such that following them will, in my humble opinion of course, improve "OK" code to "good" code.

I'm going to refer to the excellent tutorial on Java Exceptions as I continue. You should definitely read and inwardly digest that material. I particularly recommend the section Unchecked Exceptions -- the Controversy.

In the throwing and handling of exceptions, it seems to me that context is everything. This is why we sometimes wrap one exception in another -- because it allows us to add context. But there's another reason to wrap an exception. Here's a common situation:

import java.security.GeneralSecurityException;
import java.sql.Blob;
import java.sql.SQLException;

import javax.crypto.Cipher;

import org.apache.derby.iapi.jdbc.BrokeredConnection;
import org.apache.derby.iapi.jdbc.BrokeredConnectionControl;

public class MyConnection extends BrokeredConnection {

 public MyConnection(final BrokeredConnectionControl bcc, final Cipher cipher) throws SQLException {
  super(bcc);
  this.cipher = cipher;
 }

 public Blob createBlob() throws SQLException {
  return new EncryptedBlob(this.cipher) {

   public byte[] getBytes(final long pos, final int length) throws SQLException {
    final byte[] data = new byte[length];
    // Fill in the actual data from somewhere
    try {
 return this.cipher.doFinal(data, (int) pos, 0);
    } catch (final GeneralSecurityException e) {
 throw new SQLException("crypto problem with cipher "+this.cipher
   + ", length: " + length, e);
    }
   }
  };
 }

 Cipher cipher;
}

public abstract class EncryptedBlob implements Blob {

 protected Cipher cipher;

 /**
  * @param cipher the cipher to use for encryption/decryption
  * 
  */
 public EncryptedBlob(Cipher cipher) {
  super();
  this.cipher = cipher;
 }

 public long length() throws SQLException {
  return 0;
 }

        // etc. etc.
}

Here, we are extending the (Apache) Derby Connection implementation to allow for encryption/decryption of blobs. The details aren't important. But note the signature of the getBytes() method in the blob implementation. It throws a SQLException. But when we try to perform encryption, we are going to have to deal with a GeneralSecurityException. We have no choice about whether to catch or specify: we must catch it. We could eat the exception but that wouldn't be very good (see previous blog)! But since we have the ability to throw a SQLException, we will do just that: wrap the caught exception inside a SQLException. This of course also gives us the opportunity to provide some context: in this case, we don't want to pass back actual data which would be potentially insecure but, since the cipher details and the length of the byte array are quite likely to be relevant, we add those in.

What happens when we are implementing a method that doesn't throw an exception? An example of this is in the ActionListener interface, the actionPerformed(ActionEvent e) method.

 new ActionListener() {
   
  public void actionPerformed(ActionEvent e) {
   // call method that throws a checked exception  
   }
  };

The problem here is that we can't specify the exception in the method signature and we can't wrap the exception in a checked exception. We have to either handle it somehow, or throw it as a RuntimeException. That's OK. It essentially will therefore treat whatever exception is thrown as a programming (logic) error. In other words, by the time the exception bubbles up to a potential handler, we really won't be able to handle it unless we specifically catch RuntimeExceptions. And then we would have to look to see if the cause was possible to be handled. This doesn't really make good sense.

However, I should also note that, as beauty is in the eye of the beholder, so too an exception is a programming (logic) error according to the programmer. Suppose you parsing a String as a Number. If the String was created by code, then the exception is probably justified as a RuntimeException (which it is: NumberFormatException extends RuntimeException). But what if this String was read from a file or the user just typed it in. That's not a logic error. Now, we want it to be a checked exception because we must handle it somehow. So, I'm not sure that the line between checked and unchecked exceptions is quite as clear as the tutorial suggests. In this particular case, for example, the Java designers seem to have got it wrong.

If you find yourself wrapping an exception of type A in a new exception of type A, then you almost certainly shouldn't be doing it. You wrap for necessity (as above) or context. Unless there's significant context to add, you should probably leave the exception alone and let it be passed up the stack.

Now, I want to talk about handling exceptions by logging. Let's say you decide to catch an exception rather than passing it up as is. You must handle the exception by one of the following:
  • performing some other logic based on the information that an exception was thrown [for example converting a String to a Number: you first call Integer.parse(x) and if that throws a NumberFormatException  you instead try Double.parse(x)].
  • wrapping it in a new exception (as described above).
  • logging it.
In the last case, you are asserting that it's OK to continue. Meanwhile, for the purpose of improving the product you have kept a record of the incident and, if it was caused by a bug you have, in the logs, the stack trace to help in debugging.

But you shouldn't do more than one of these things. Think of it this way: there shouldn't be more than one reference to the exception. We should never, for example, find an exception has been logged twice. In the following code, the try/catch in doMain() is completely unnecessary. And it results in two copies of the exception going forward. Bad practice.
public class X {

 public void doSomethingUseful() throws Exception {
  throw new Exception("problem");
 }

 protected void doMain() throws Exception {
  try {
   doSomethingUseful();
  } catch (final Exception e) {
   logger.log(Level.WARNING, "something bad", e);
   throw new Exception("wrapped", e);
  }
 }

 private static Logger logger = Logger.getLogger(X.class.getName());

 public static void main(final String[] args) {
  final X x = new X();
  try {
   x.doMain();
  } catch (final Exception e) {
   e.printStackTrace();
  }
 }
}

If you are writing UI code and an exception bubbles up from below, then it makes sense to do the following:
  1. log it; and
  2. if appropriate, tell the user what went wrong (in terms the user will understand, which is generally not the way exceptions are created) and what he/she can do about it.
Finally, I want to strongly suggest the following rules (which I will not attempt to justify):

  • There should be no more than one try/catch/finally block in any one method. Parallel to this rule is that there should be no more than one loop clause in a method (and ideally only one if construct).
  • Don't bother to catch any exceptions in a private method unless you are going to do something really useful with it.
  • As much as possible (Java 1.7 is good here) bunch the catch clauses together.
  • Be careful only to catch the types of exception that you really want to handle (and can handle) -- don't for example specify Exception in a catch clause because you will end up catching RuntimeExceptions and you won't know if it's safe to proceed. It's OK to catch a superclass (e.g. GeneralSecurityException) but Exception is just too generic.
  • User try-with-resource when appropriate (Java 1.7).
OK, back to work!

Friday, April 18, 2014

Reactive Programming

LinkedIn invited me to write a blog on the site (I don't know if this is a special privilege or the ask everyone) but I wanted to write about Reactive Programming. I have a friend who is looking for work and has been around the block in the software industry for a long time (we're the same age). So, I was able to use his situation as the seed for the idea.

If you'd like to read it, here it is.

OK, back to work!

Friday, October 19, 2012

More ways to try to maximize quality

Yesterday we looked at unit testing, especially test-driven development.  I'm going to add one more type of test here: a test for a private method (or any method which is otherwise out of scope).

Take a look at these two methods in the test class:
 @Test
 public void getTotal_A$() throws Exception {
  final Randomizer target = new Randomizer(getRandom());
  assertEquals(0, getTotal(target));
  target.setBands(1, 2, 3, 4);
  assertEquals(10, getTotal(target));
 }

 private int getTotal(final Randomizer target) throws Exception {
  Object total = TestUtilities.invokeInstanceMethod(target, "getTotal", new Class[] {}, new Object[] {}); //$NON-NLS-1$
  assertTrue(total instanceof Integer);
  return ((Integer) total).intValue();
 }


Most of the time, we can probably skip testing private methods.  But there are times when the private method contains some especially tricky logic, which is then further complicated by its caller(s) and we want to isolate the test to focus on precisely the private method.  I use a utility method which is in a class called TestUtilities which I put in the test directory along with the unit test classes.  The particular method used here is simple enough - I'll just show it in-lined (although the class actually has a number of methods with convenient signatures and is rather more elegant).
 public static Object invokeInstanceMethod(final Object instance, final String methodName, final Class[] paramClasses,
   final Object[] paramObjects) throws Exception {
  final Method method = instance.getClass().getDeclaredMethod(methodName, paramClasses);
  method.setAccessible(true);
  return method.invoke(instance, paramObjects);


Now let's take a look at some tools to try to improve code quality.  In a previous blog, I extolled the virtues of cleaning up code (using the cleanup tool in Eclipse, for example) before commiting to source control. However, perhaps I didn't sufficiently emphasize that it is not just formatting that is significant, but code cleanup. So, let's assume that our code has been cleaned up by Eclipse (or whatever) and compiles and passes all of its unit tests.

First, I want to take a look at the dreaded NullPointerException.  You definitely want to turn on the Java compiler warning which warns that this is possible ("Null pointer access" under "Potential programming problems"). This can find some possible problems, but it's not nearly as good in Eclipse Indigo as it is in Juno (Eclipse version 4).  Juno has annotations to allow you to explicitly set the expected behavior of references.  There are also the
javax.validation.constraints annotations like @NotNull though I'm not sure how useful these really in practice.  Until we can adopt the Juno annotations, we can try to avoid using variables (and initializing them to null) when we should be using constants.  And, unless it is explicitly covered as a potential and valid return value, methods should never return null.  If null is an invalid response it is better to throw an exception.  Assertions can also be used (by specying the -ea compiler option), but these have always seemed a crude way of doing things.  Annotations are much more appropriate.

I like another Eclipse plugin called FindBugs.  This will run on selected code (whole projects if you like) and come up with some potential bugs.  In this case, it takes mild exception to the test class, generating 7 warnings like the following:
RandomizerTest.java:54 Random object created and used only once in RandomizerTest.nextBoolean_A$()
This is not particularly helpful because we knew that already - this is the test class, remember?  Strangely, it doesn't seem to care that there are about a dozen potential null-pointer exceptions.  Occasionally, it will complain about something that you want to keep as is.  There is generally an annotation that you can specify which will tell it not to "bug" you about that in the future (similar to the annotations that you can use to switch off specific compiler warnings).
Then finally, when you've fixed all of the warnings that the compiler gives, and all of the bugs that FindBugs finds, and dealt with any null pointer issues, it's time to look for code "smells".  This is my favorite part.  Most of the time I just look and get that feeling that something's, well, fishy.  But you can do it more scientifically.  The tool I'm most familiar with, another Eclipse plugin, is called JDeodorant.

It has a strange user interface that I really dislike but it does work and can detect four "flavors" of code smell: God class, Long method, Type checking and Feature envy. Even though my example is quite short and reasonably well programmed, JDeodorant does find a few odors and with recommendations for fixing, for example under God class, it does suggest a refactoring and similarly under Long method.  It will offer to refactor the code for you.  This is all well and good if the changes are just to one file because you can revert to a previous version if you like.  But be careful if several files will be refactored, as feature envy often suggests.  I would only use that after checking everything else into source control.

Going beyond the level of source code, of course there are profiles and other performance enhancing tools but these are beyond the scope of this article.  There is one other tool that I like to help maintain architectural integrity of packages (packages are perhaps the least well thought-out aspect of Java).  It is called SonarJ and can really help with getting your house in order, architecturally speaking, by detecting cycles (A calls B, B calls C, C calls A, etc.).  It will recommend how to break these cycles by introducing interfaces.  Even a relatively simple package (or set of packages) can be surprisingly complex - and you really need a good tool to keep things in order.
OK, back to work!

Thursday, October 18, 2012

Test-driven development

To my mind, the most important software develoment tool, next to the compiler, I suppose, is a unit test framework.  I wrote one of these back in 1985 and I regret that I couldn't persuade the company to develop it further, perhaps even replacing our actual product.  But back in those days, the other tools just weren't ready for unit testing -- everything took far too long and we didn't have anything like continuous integration or even an IDE.  There was a powerful Q/A lobby which saw unit testing as trying to put them out of work!

Fast forward to 2012 and not only do we have unit testing, we have mockers and test-code generators.  I will admit right now that I am not an expert (barely a novice) in the realm of mocking.  It may be that some of what I am about to describe could be done better a different way.  No matter, onward.

I like to use a little tool called JUnitHelper which is an Eclipse plugin.  Now, it certainly has its quirks and idiosyncracies.  But it does do a basic first cut at a set of methods that "cover" all of the (non-private) methods declared in a source file.  And it makes it easy to hop back and forth between test class and test target class (Alt 9 and Alt 8).  When you enter Alt 9 (you can of course do it via menu commands too) it will add any test methods that have become necessary since you last did an Alt 9.  If there is no test class at all (defined by adding "Test" to type name), it will create one for you.  There are options so you can vary (a little) the various things that it does for you.

So here's what it produces for the Randomizer class that we talked about in the last blog (assuming we have it set to generate JUnit 4 tests):
package phasmid;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.junit.Assert.assertThat;

import org.apache.commons.math3.random.RandomGenerator;
import org.junit.Test;

public class RandomizerTest {

 @Test
 public void type() throws Exception {
  // TODO auto-generated by JUnit Helper.
  assertThat(Randomizer.class, notNullValue());
 }

 @Test
 public void instantiation() throws Exception {
  // TODO auto-generated by JUnit Helper.
  RandomGenerator random = null;
  Randomizer target = new Randomizer(random);
  assertThat(target, notNullValue());
 }

 @Test
 public void nextBoolean_A$() throws Exception {
  // TODO auto-generated by JUnit Helper.
  RandomGenerator random = null;
  Randomizer target = new Randomizer(random);
  boolean actual = target.nextBoolean();
  boolean expected = false;
  assertThat(actual, is(equalTo(expected)));
 }
etc. etc.....
I'm not in love with having those // TODO lines added but I suppose that's their way of advertising.  I'd prefer it to use more readable assertions such as:
  assertNotNull(Randomizer.class);

  instead of
  assertThat(Randomizer.class, notNullValue());


or 
  boolean actual = target.nextBoolean();
  assertFalse(actual);

  instead of


  boolean actual = target.nextBoolean();
  boolean expected = false;
  assertThat(actual, is(equalTo(expected)));

  or, better yet,
  assertFalse(target.nextBoolean());


But these details aren't too significant.  Another possible improvement might be to create a @Before setup method which reinitializes a field to a given state before each test.  Then you would, typically, only need to have one initializer for each field, instead of an initializer per field per method.  But, again, small potatoes.

I will actually create that method just so that the code is a little more readable.

So, now let's get to some real work.  We want to be able to set an array of relative frequencies for the various "bands" of the wheel of fortune.  So we create a test method to set those bands.  We will also create a test method to get the next index.  We will try to name these tests according to the scheme specified in the options for JUnitHelper (but if we don't, it won't be the end of the world, we will just ultimately end up with two test methods for one target method).

This is how the start of our test class looks now:
 private RandomGenerator random;

 @Before
 public void setup() {
  setRandom(new JDKRandomGenerator());
  getRandom().setSeed(0);
 }
 
 @Test
 public void setBands_A$() {
  Randomizer target = new Randomizer(getRandom());
  target.setBands(new int[]{1,2});
 }

 @Test
 public void nextIndex_A$() {
  Randomizer target = new Randomizer(getRandom());
  int actual = target.nextIndex();
 }

 @Test
 public void type() throws Exception {
  assertNotNull(Randomizer.class);
 }


Errors appear for setBands(...) and nextIndex() because these methods don't yet exist (so you also don't get any auto-complete when typing those names, obviously).

So, we opt for the appropriate quick fix: create the methods in the target class. But before fleshing these new method bodies out, let's actually define the behavior that we want right now.

We want the default behavior (if no bands are set) of nextIndex() to match what nextInt() would return if its parameter was Integer.MAX_VALUE.  So, let's code that in our test method for nextIndex.  But we will also add logic to test the invocation of setBands() and a subsequent call to getIndex() [actually, we will make many such calls].

We also check that the delegated methods behave as expected.

This is what (the start of) our test class looks like now:
 private static final int SEED = 0;

 private RandomGenerator random;

 @Before
 public void setup() {
  setRandom(new JDKRandomGenerator());
  getRandom().setSeed(SEED);
 }

 @Test
 public void setBands_A$() {
  Randomizer target = new Randomizer(getRandom());
  target.setBands(new int[] { 1, 2 });
  int[] counts = new int[] { 0, 0};
  final int n = 100000;
  for (int i = 0; i < n; i++) {
   int nextIndex = target.nextIndex();
   assertTrue(nextIndex == 1 || nextIndex == 0);
   counts[nextIndex]++;
  }
  assertTrue(counts[0] + counts[1] == n);
  assertTrue(Math.abs(counts[1] - 2 * counts[0]) < 500);
 }

 @Test
 public void nextIndex_A$() {
  Randomizer target = new Randomizer(getRandom());
  int actual = target.nextIndex();
  getRandom().setSeed(SEED);
  int nextInt = target.nextInt(Integer.MAX_VALUE);
  assertEquals(nextInt, actual);
 }

 @Test
 public void type() throws Exception {
  assertNotNull(Randomizer.class);
 }

 @Test
 public void instantiation() throws Exception {
  Randomizer target = new Randomizer(getRandom());
  assertThat(target, notNullValue());
 }

 @Test
 public void nextBoolean_A$() throws Exception {
  Randomizer target = new Randomizer(getRandom());
  final boolean actual = target.nextBoolean();
  boolean expected = new Random(SEED).nextBoolean();
  assertEquals(expected, actual);
 }

 @Test
 public void nextBytes_A$byteArray() throws Exception {
  Randomizer target = new Randomizer(getRandom());
  byte[] actual = new byte[] {};
  target.nextBytes(actual);
  byte[] expected = new byte[] {};
  new Random(SEED).nextBytes(expected);
  assertTrue(Arrays.equals(expected, actual));
 }
Notice that we've completely define the desired behavior of the setBands() and nextIndex() methods but we still haven't actually coded the case where there are bands set.  Not surprisingly, the unit test runs excatp that there is an assertion error in the setBands_A$() method.

Wouldn't it be nice if we could get that written for us.  Well, on second thoughts, wouldn't we be out of a job then?  Actually, there are methods to create such conforming code, via evolutionary programming but for now, we'll just do it our own way.

This is what we end up with for the two methods in question:
 /**
  * @param bands
  *            a set of relative frequencies for the index values 0, 1, ...
  */
 public void setBands(int... bands) {
  this._bands = bands;
  int count = 0;
  for (int i = 0; i < getBands().length; i++)
   count += getBands()[i];
  setTotal(count);
 }

 /**
  * The behavior of this method depends on whether any bands were defined for
  * this randomizer. If bands were defined, the result is an index into the
  * bands. If bands were not defined, the result is a number uniformly
  * distributed between 0 and {@link Integer#MAX_VALUE}. In both cases, the
  * superclass method {@link RandomGenerator#nextInt(int)} is called.
  * 
  * @return the "index" as defined above.
  */
 public int nextIndex() {
  if (getTotal() > 0) {
   int randomizer = nextInt(getTotal());
   for (int i = 0; i < getBands().length; i++) {
    randomizer -= getBands()[i];
    if (randomizer < 0)
     return i;
   }
   return -1; // logic error
  }

  return nextInt(Integer.MAX_VALUE);
 }


Of couse it isn't necessary to do test-driven development as shown above.  It's perfectly valid to write the method and then do the testing.  But it can actually be easier and more obvious what's going on when you do it this way.  And, moreover, your unit tests then actually become documentation on how your methods should be used.

Finally, you need to test for coverage.  The full suite of tests that I've defined above is not really a complete set.  There is a lot more we could test for.  But let's see what sort of coverage we get, by running EclEmma, another Eclipse plugin that I find works very well.  You might prefer Cobertura or one of the others.  Come to think of it you might prefer other test runners than JUnit.

EclEmma tells us that things aren't too bad: 356 covered instructions, 4 missed.  I think we could do rather more testing of the nextIndex() method, perhaps using a greater number of trials than 100,000, maybe with a greater number of bands.

Tomorrow we're going to look at unit testing private methods and also search for bugs and code smells.

OK, back to work!

Monday, October 15, 2012

My process for developing software: creating a simple class

This is the first of several blogs that outline the way I like to develop (or modify) code. I'm talking in particular about Java development using Eclipse, although most of what I'm saying should be generally applicable.

In just about every phase of development, I avoid typing as much as is possible.  Not only is typing relatively slow (although I'm fairly quick myself) but it is error-prone.

And bear in mind that this is a personal view - I don't expect everyone to agree with it in every detail.  Also, I am going to discuss certain tools but there are usually alternatives to those specific tools that do something similar.  For example, I am using Eclipse Indigo.  I haven't manage to get Juno to behave properly yet.  And you may prefer IntelliJ or something else.

So, let's get started.

For now, let's assume that you are developing a new class (or class family that implements a new interface) and you're starting pretty much from scratch.

Before I do anything else, I naturally search the web for something similar.  Anything that is some sort of utility code has almost certainly been done by someone else somewhere.  This is especially true of anything to do with Collections.

So, let's assume that we couldn't find anything suitable (or we just didn't like what there was).  As an example, we're going to develop a "wheel of fortune" class which we will call Randomizer.

Unfortunately, in the early days of Java, they didn't "eat their own dog food" and did not create interfaces to define many of the standard classes.  java.util.Random is a case in point.  So the good people of Apache created an interface called RandomGenerator which does define the methods we want.

So, we will start by creating a new class Randomizer and have it implement RandomGenerator and Serializable.  But we will arrange for it to delegate its random number generation duties.

In the spirit of avoiding typing I try to include things like comments and constructors.  But I never want a main program (well, maybe once in every few hundred classes) and, because we're going to delegate the random number generation responsibilities, we will not for now implement the inherited abstract methods.

This is what results:


/**
 * Phasmid blog sandbox
 * 
 * Module: Randomizer
 * Created on: Oct 15, 2012
 */

package phasmid;

import java.io.Serializable;

import org.apache.commons.math.random.RandomGenerator;

/**
 * @author phasmid
 * 
 */
public class Randomizer implements RandomGenerator, Serializable {

 /**
  * 
  */
 public Randomizer() {
  // XXX Auto-generated constructor stub
 }

}

Randomizer is underscored in red in Eclipse because it doesn't implement those abstract methods.  Let's talk about warnings now.  I have Eclipse give me just about every possible warning.  Everything which by default is ignored, I change to warn.

So, first we go to the constructor and replace the comment with:

super();


Yes, I know that it isn't necessary but, to me, a constructor without anything in it looks naked.  Now we click on the constructor and invoke the Change Method Signature refactoring (Alt/Shift/C in Eclipse).  We're going to add a parameter called random of type RandomGenerator.  The default value doesn't matter because nothing calls the constructor yet so null will do.  But if the constructor is in use, I usually try to name the parameter with the same name - that might just match the appropriate identifier in the caller or, if not, it will at least be clear what's needed.  I'm all for naming identifiers according to context, but something that might be passed from one constructor to the next will usually only need one universal name.

At this point, the parameter name should be underscored in yellow (a warning showing that this is an unused parameter in a non-overridden method).  [In the Preferences->Java->Compiler->Errors/Warnings->Unnecessary dode->Value of parameter is not used, I have unchecked the checkbox for Ignore parameters documented with @param tag.

So now I click on the parameter name (random) and invoke the quick-fix (Ctrl/1), selecting the Assign parameter to new field option.  For some reason I don't understand, the options you get when you hover over the warning do not include this most obvious solution.  The quick fixes are like that - very inconsistent and frequently missing the most obvious solution.

So, with a (private and final) field named _random defined, this is what the contents of the class look like now.

 private final RandomGenerator _random;

 /**
  * @param random
  *            XXX
  * 
  */
 public Randomizer(RandomGenerator random) {
  super();
  _random = random;
 }

We will eventually clean this up but not just yet.  We've got warnings (underscored here) and we are going to address those immediately.

We click on _random and invoke the Source -> Generate Delegate Methods option (Alt/Shift/S).  This fills in all of our delegate methods and, since the field implements the same interface as the class, our error goes away.

But we end up with some warnings for every reference to _random.  This is because it is an instance field and we have not prefixed the references with "this."  Up until a few years ago, I always wanted to suppress those qualifiers.  But now, it seems to me that they add some clarity and do no harm.  Here's an Eclipse annoyance: if we choose the quick fix "create getter and setter", it works well.  But if you later add more references, you can't convert them all to use the getter at the same time.  BTW, I make the getRandom() have private scope since it would be overkill to have the class implement the interface and have public access to a field that does the same thing.

We're left with three warnings: two references to _random which need to be qualified.  And the fact that we haven't got a serial UID.  All three of these problems can be easily fixed with quick fixes.

A bit of (automatic) cleanup [we'll talk more about this later] and we have the following code:
[starting with]

 /**
  * @param random
  *            XXX
  * 
  */
 public Randomizer(final RandomGenerator random) {
  super();
  this._random = random;
 }

 /**
  * {@inheritDoc}
  * 
  * @see org.apache.commons.math.random.RandomGenerator#nextBoolean()
  */
 @Override
 public boolean nextBoolean() {
  return getRandom().nextBoolean();
 }


[and ending with]

 /**
  * {@inheritDoc}
  * 
  * @see org.apache.commons.math.random.RandomGenerator#setSeed(long)
  */
 @Override
 public void setSeed(final long arg0) {
  getRandom().setSeed(arg0);
 }

 /**
  * @return the random
  */
 private RandomGenerator getRandom() {
  return this._random;
 }

 private static final long serialVersionUID = 1238102722233258232L;

 private final RandomGenerator _random;



Next blog: test-driven development.

OK, back to work!