NetBeans

I was very excited to start using Infinitest once again. But as things seem to go with me, I ended up moving on to something else that dominated my mindshare–in this case, a new project where I am using NetBeans. Why NetBeans? Because someone set up the project to use Maven, and all I’ve heard is that NetBeans is magical when it comes to integrating with Maven (and Eclipse, not so much).

As I would expect, both Maven and Maven support in NetBeans are really nice as long as they’re working well. But Maven seems to adhere to the 80/20 rule. It’s great 80% of the time, up until you have to do something a little out of the ordinary, or when things aren’t working like they’re supposed to. Then the numbers reverse–you spend 80% of your time trying to work on the remaining 20%. In the case of Maven, I’m struggling with getting coverage reports against multiple modules, and it’s also presenting me with a defect (operator error?) where NetBeans reports half the Maven modules as “badly formed Maven projects.” Google searches, and assistance from others with more Maven experience than me, have not been very fruitful.

This is the third time I’ve used Maven, and I’m warming up to it more each time, but it’s still been a frustration. Part of frustration is always lack of knowledge, particularly around the behind-the-scenes workings that you need to know when you hit the 20% esoterica. Thanks to those who have helped!

On the other hand, this is also the third time I’ve used NetBeans heavily. I’m not really warming to it much. Granted, part of my disappointment with NetBeans is the same problem. The best way to learn not to hate something is to pair with someone who knows it well, and I’ve not been able to get much pairing time.

Other than the Maven support, NetBeans seems much weaker than Eclipse. It’s slower (e.g. a Ctrl-O to search for types takes many seconds to return class names, compared to Eclipse’s Ctrl-Shift-t which is almost immediate). It’s flakier (e.g. half the time, ctrl-F6 to run a test pops up its JUnit GUI, the other half the time I see only the console output; also, the formatting mechanism appears to be broken). It seems to be the last of the three main Java IDEs for which people write plugins (e.g. Infinitest is only available for Eclipse and IDEA, not NetBeans). And it’s just missing key important things (e.g. there is no inline method refactoring until NetBeans 7.0).

I can live without every last bell and whistle. But it’s always disappointing to see other people whistle in cool little ways, like Kerievsky’s nice trick with inline method, and not get to play along.

Comments:

There is no way to read the Kerievsky trick without becoming a member of the Yahoo group. It would be helpful to cut and paste the content.
# posted by Anonymous Anonymous : 5/08/2010 12:59:00 AM
Aug 5, 2009, from Joshua K:

One of the refactoring strategies I defined and use quite a bit is called Parallel Change. You put new behavior next to old behavior (wherever it is) and then do a Gradual Cutover from old to new. When you’re done cutting
over to the new code, you need to get rid of the old code.

Say you’ve got some old method that was called in 10 places in a class. The new code, which had been placed under each of the 10 calls to the old code, is now doing the real work. You don’t need the old method and the calls to it.

In the past, I would delete the method, then follow the chain of red marks to delete each call to the old method. That was before a smart student pointed out a better way:

* Delete the body of the old method
* Inline the old method – effectively inlining “nothing”

That approach is much faster and far more satisfying.

Is this something folks already do? It had not occurred to me to use inline in that way to delete code.

This does go along with an observation I’ve made that the better you get at refactoring, the more you use the Inline Method refactoring.
posted by Blogger Jeff L. : 5/10/2010 07:44:00 AM

Easier Testing Using the SRP

This simple file monitor class notifies listeners when a watched file is modified:

public class FileMonitor {
 private Set<FileChangeListener> listeners =
    new HashSet<FileChangeListener>();
 private int duration;
 private File file;
 private long lastModified;

 public FileMonitor(String filename, int duration) {
     this.file = new File(filename);
     lastModified = file.lastModified();
     this.duration = duration;
 }

 public void start() {
     boolean isDaemon = true;
     long initialDelay = duration;
     new Timer(isDaemon).schedule(
        new FileMonitorTask(), initialDelay, duration);
 }

 public void addListener(FileChangeListener listener) {
     listeners.add(listener);
 }

 class FileMonitorTask extends TimerTask {
     @Override
     public void run() {
         if (file.lastModified() > lastModified) {
             lastModified = file.lastModified();
             for (FileChangeListener listener: listeners) {
                 listener.modified();
             }
         }
     }
  }
}

A FileMonitor schedules a TimerTask that just checks the modified date from time to time, and compares it to the last modified date. The code above seems like a typical and reasonable implementation. FileMonitorTask could be an anonymous inner class, of course. I spent more time than I would have preferred test-driving this solution, since I made the mistake of test-driving it as a single unit. These tests had to deal with the vagaries of threading due to the Timer scheduling.

import java.io.*;
import java.util.*;
import org.junit.*;
import static org.junit.Assert.*;

public class FileMonitorTest {
 private static final String FILENAME = "FileMonitorTest.properties";
 private static final int MONITOR_DURATION = 1;
 private static final int TIMEOUT = 50;
 private FileMonitor monitor;
 private File file;
 private List<FileChangeListener> fileChangeNotifications;
 private Thread waitThread;

 @Before
 public void initializeListenerCounter() {
     fileChangeNotifications = new ArrayList<FileChangeListener>();
 }

 @Before
 public void createFileAndMonitor() throws IOException {
     FileUtils.writeFile(FILENAME, "property=originalValue");
     file = new File(FILENAME);
     monitor = new FileMonitor(FILENAME, MONITOR_DURATION);
 }

 @After
 public void deleteFile() {
     FileUtils.deleteIfExists(FILENAME);
 }

 @Test
 public void shouldNotifyWhenFileModified() throws Exception {
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(1);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();
     assertEquals(1, numberOfFileChangeNotifications());
 }

 @Test
 public void shouldSupportMultipleListeners() throws Exception {
     monitor.addListener(createCountingFileChangeListener());
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(2);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();
     assertEquals(2, numberOfFileChangeNotifications());
     assertAllListenersDistinctlyNotified();
 }

 @Test
 public void shouldOnlyReportOnceForSingleModification()
         throws InterruptedException {
     // slow--must wait on timeout. Better way?
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(2);

     monitor.start();

     alterFileToChangeLastModified();

     joinWaitThread();
     assertEquals(1, numberOfFileChangeNotifications());
 }

 @Test
 public void shouldReportMultipleTimesForMultipleModification()
     throws InterruptedException {
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(1);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();

     waitForFileChangeNotifications(2);
     alterFileToChangeLastModified();
     joinWaitThread();

     assertEquals(2, numberOfFileChangeNotifications());
 }

 private FileChangeListener createCountingFileChangeListener() {
     return new FileChangeListener() {
         @Override
         public void modified() {
             fileChangeNotifications.add(this);
         }
     };
 }

 private int numberOfFileChangeNotifications() {
     return fileChangeNotifications.size();
 }

 private void waitForFileChangeNotifications(final int expected) {
     waitThread = new Thread(new Runnable() {
         @Override
         public void run() {
             while (numberOfFileChangeNotifications() != expected)             {
                 sleep(1);
             }
         }
     });
     waitThread.start();
 }

 private void sleep(int ms) {
     try {
         Thread.sleep(ms);
     } catch (InterruptedException exception) {
         fail(exception.getMessage());
     }
 }

 private void alterFileToChangeLastModified() {
     file.setLastModified(file.lastModified() + 1000);
 }

 private void joinWaitThread() throws InterruptedException {
     waitThread.join(TIMEOUT);
 }

 private void assertAllListenersDistinctlyNotified() {
     Set<FileChangeListener> notifications =
         new HashSet<FileChangeListener>();
     notifications.addAll(fileChangeNotifications);
     assertEquals(fileChangeNotifications.size(), 
                  notifications.size());
 }
}

Wow, that’s a bit longer than I would have hoped. Yes, there are better ways to test the threading. And there may be better threading mechanisms to implement the monitor, although I think the Timer is pretty simple. I don’t do enough threading, so I usually code a “familiar” solution (i.e. something I already know well) initially, and then refactor into a more expressive and concise solution. I’m pretty sure this test is flawed. But rather than invest in fixing it, I reconsidered my design (by virtue of wanting an easier way to test it).

The SUT is really two classes, the monitor and the task. Each one can easily be tested separately, in isolation, as a unit. Sure, I want to see the threading in action, but perhaps that’s better relegated to an integration test (which might also double as an acceptance test). I decided to change the solution to accommodate more focused and more robust tests. The file monitor tests are pretty much what was there before (with some small additional refactorings), except that there’s now no concern over threading–I just test the run method:

import java.io.*;
import org.junit.*;
import static org.mockito.Mockito.*;
import org.mockito.*;

public class FileMonitor_TaskTest {
 private static final String FILENAME = "FileMonitorTest.properties";
 private FileMonitor.Task task = new FileMonitor.Task(FILENAME);
 @Mock
 private FileChangeListener listener;

 @Before
 public void initializeMockito() {
     MockitoAnnotations.initMocks(this);
 }

 @BeforeClass
 public static void createFile() throws IOException {
     FileUtils.writeFile(FILENAME, "property=originalValue");
 }

 @AfterClass
 public static void deleteFile() {
     FileUtils.deleteIfExists(FILENAME);
 }

 @Before
 public void createTask() {
     task = new FileMonitor.Task(FILENAME);
 }

 @Test
 public void shouldNotNotifyWhenFileNotModified() {
     task.addListener(listener);
     task.run();
     verify(listener, never()).modified();
 }

 @Test
 public void shouldNotifyWhenFileModified() throws Exception {
     task.addListener(listener);
     changeFileLastModifiedTime();

     task.run();

     verify(listener, times(1)).modified();
 }

 @Test
 public void shouldSupportMultipleListeners() throws Exception {
     task.addListener(listener);
     FileChangeListener secondListener = mock(FileChangeListener.class);
     task.addListener(secondListener);

     changeFileLastModifiedTime();
     task.run();

     verify(listener, times(1)).modified();
     verify(secondListener, times(1)).modified();
 }

 @Test
 public void shouldOnlyReportOnceForSingleModification() 
         throws InterruptedException {
     task.addListener(listener);

     task.run();
     changeFileLastModifiedTime();
     task.run();
     task.run();

     verify(listener, times(1)).modified();
 }

 @Test
 public void shouldReportMultipleTimesForMultipleModification() 
          throws InterruptedException {
     task.addListener(listener);

     task.run();
     changeFileLastModifiedTime();
     task.run();
     changeFileLastModifiedTime();
     task.run();

     verify(listener, times(2)).modified();
 }

 private void changeFileLastModifiedTime() {
     File file = new File(FILENAME);
     file.setLastModified(file.lastModified() + 1000);
 }
}

I introduced Mockito to provide a simple verifying stub for the listener. I suppose I should also stub out interactions with File, but I’ll incur the speed penalty for now. Next, I needed to test the remaining FileMonitor code. To do this involved proving that it creates and schedules a task appropriately using a timer, and that it handles listeners appropriately.

import xxx.CollectionUtil;
import java.util.Timer;
import org.junit.*;
import org.mockito.*;
import static org.mockito.Mockito.*;

public class FileMonitorTest {
 @Mock
 private Timer timer;

 @Before
 public void initializeMockito() {
     MockitoAnnotations.initMocks(this);
 }

 @Test
 public void shouldScheduleTimerWhenStarted() {
     String filename = "filename";
     long duration = 10;
     FileMonitor monitor = new FileMonitor(filename, duration) {
         @Override
         protected Timer createTimer() {
             return timer;
         }
     };
     monitor.start();
     verify(timer).schedule(any(FileMonitor.Task.class), 
         eq(duration), eq(duration));
 }

 @Test
 public void shouldDelegateListenersToTask() {
     FileChangeListener listener = mock(FileChangeListener.class);
     FileMonitor monitor = new FileMonitor("", 0);
     monitor.addListener(listener);

     CollectionUtil.assertContains(
         monitor.getTask().getListeners(), listener);
  }
}

The design of the production code had to change based on my interest in better tests. A number of small, incremental refactoring steps led me to this solution:

import java.io.*;
import java.util.*;

public class FileMonitor {
 private final long duration;
 private Task task;

 public FileMonitor(String filename, long durationInMs) {
     task = new Task(filename);
     this.duration = durationInMs;
 }

 public void start() {
     long initialDelay = duration;
     createTimer().schedule(task, initialDelay, duration);
 }

 protected Timer createTimer() {
     final boolean isDaemon = true;
     return new Timer(isDaemon);
 }

 Task getTask() {
     return task;
 }

 public void addListener(FileChangeListener listener) {
     task.addListener(listener);
 }

 static class Task extends TimerTask {
     private final File file;
     private long lastModified;
     private Set<FileChangeListener> listeners = 
         new HashSet<FileChangeListener>();

     Task(String filename) {
         this.file = new File(filename);
         lastModified = file.lastModified();
     }

     public void addListener(FileChangeListener listener) {
         listeners.add(listener);
     }

     Set&lt;FileChangeListener&gt; getListeners() {
         return listeners;
     }

     @Override
     public void run() {
         if (file.lastModified() > lastModified) {
             lastModified = file.lastModified();
             for (FileChangeListener listener: listeners) {
                 listener.modified();
             }
         }
     }
 }
}

Well, most interestingly, I no longer have any tests that must deal with additional threads. I test that the monitor delegates appropriately, and I test that the task works properly when run. I still want the integration tests, but I think they can be relegated to a higher, acceptance level test for the story:

reload properties at runtime

Otherwise, at this point, I have a high level of confidence in my code. I know the Timer class works, and I know my two classes work, and I’ve also demonstrated to myself that they all work together. I’m ready to move on. Some additional relevant points:

  • Threading is tough to get right, and it can be even tougher to test it.
  • My final solution is a few more lines of code due to supporting my interest in more focused testing. I’m ok with the additional code because of the flexibility having the tests gives me, and more often than not, having them allows me to easily eliminate duplication elsewhere.
  • It’s hard to be overdo the SRP. Far more classes than not weigh in the wrong direction of too many responsibilities.
  • The original solution, probably fine otherwise, is what you might expect from someone uninterested in TDD. It works (I think) but it represents rigid code, code that is more costly to change.

 

Atom