diff --git a/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java b/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
index 569a922..c6d730d 100644
--- a/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
+++ b/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
@@ -2,15 +2,14 @@
 
 import org.codehaus.jackson.annotate.JsonProperty;
 
-
 public class ControllerRegistryEntry implements Comparable<ControllerRegistryEntry> {
     //
     // TODO: Refactor the implementation and decide whether controllerId
     // is needed. If "yes", we might need to consider it inside the
-    // compareTo(), equals() and hashCode() impmenetations.
+    // compareTo(), equals() and hashCode() implementations.
     //
-    private String controllerId;
-    private int sequenceNumber;
+    private final String controllerId;
+    private final int sequenceNumber;
 
     public ControllerRegistryEntry(String controllerId, int sequenceNumber) {
         this.controllerId = controllerId;
diff --git a/src/main/java/net/onrc/onos/core/registry/IdBlock.java b/src/main/java/net/onrc/onos/core/registry/IdBlock.java
index d905917..7184239 100644
--- a/src/main/java/net/onrc/onos/core/registry/IdBlock.java
+++ b/src/main/java/net/onrc/onos/core/registry/IdBlock.java
@@ -1,9 +1,9 @@
 package net.onrc.onos.core.registry;
 
 public class IdBlock {
-    private long start;
-    private long end;
-    private long size;
+    private final long start;
+    private final long end;
+    private final long size;
 
     public IdBlock(long start, long end, long size) {
         this.start = start;
diff --git a/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java b/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
index 989d2c1..de07adf 100644
--- a/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
+++ b/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
@@ -15,6 +15,7 @@
 import net.floodlightcontroller.restserver.IRestApiService;
 import net.onrc.onos.core.registry.web.RegistryWebRoutable;
 
+import org.apache.commons.lang.NotImplementedException;
 import org.openflow.util.HexString;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -23,17 +24,15 @@
  * Implementation of a registry that doesn't rely on any external registry
  * service. This is designed to be used only in single-node setups (e.g. for
  * development). All registry data is stored in local memory.
- *
- * @author jono
  */
 public class StandaloneRegistry implements IFloodlightModule,
         IControllerRegistryService {
     private static final Logger log = LoggerFactory.getLogger(StandaloneRegistry.class);
 
-    protected IRestApiService restApi;
+    private IRestApiService restApi;
 
-    protected String registeredControllerId = null;
-    protected Map<String, ControlChangeCallback> switchCallbacks;
+    private String registeredControllerId;
+    private Map<String, ControlChangeCallback> switchCallbacks;
 
     //
     // Unique ID generation state
@@ -44,7 +43,7 @@
     public void requestControl(long dpid, ControlChangeCallback cb)
             throws RegistryException {
         if (registeredControllerId == null) {
-            throw new RuntimeException(
+            throw new IllegalStateException(
                     "Must register a controller before calling requestControl");
         }
 
@@ -52,7 +51,7 @@
 
         log.debug("Control granted for {}", HexString.toHexString(dpid));
 
-        //Immediately grant request for control
+        // Immediately grant request for control
         if (cb != null) {
             cb.controlChanged(dpid, true);
         }
@@ -103,7 +102,7 @@
 
     @Override
     public String getControllerForSwitch(long dpid) throws RegistryException {
-        return (switchCallbacks.get(HexString.toHexString(dpid)) != null) ? registeredControllerId : null;
+        return (switchCallbacks.get(HexString.toHexString(dpid)) == null) ? null : registeredControllerId;
     }
 
     @Override
@@ -125,25 +124,29 @@
     @Override
     public Collection<Long> getSwitchesControlledByController(
             String controllerId) {
-        throw new RuntimeException("Not yet implemented");
+        throw new NotImplementedException("Not yet implemented");
     }
 
-    private long blockTop = 0L;
+    private long blockTop;
     private static final long BLOCK_SIZE = 0x1000000L;
 
     /**
      * Returns a block of IDs which are unique and unused.
      * Range of IDs is fixed size and is assigned incrementally as this method called.
+     *
+     * @return an IdBlock containing a set of unique IDs
      */
     @Override
-    public synchronized IdBlock allocateUniqueIdBlock() {
-        long blockHead = blockTop;
-        long blockTail = blockTop + BLOCK_SIZE;
+    public IdBlock allocateUniqueIdBlock() {
+        synchronized (this)  {
+            long blockHead = blockTop;
+            long blockTail = blockTop + BLOCK_SIZE;
 
-        IdBlock block = new IdBlock(blockHead, blockTail - 1, BLOCK_SIZE);
-        blockTop = blockTail;
+            IdBlock block = new IdBlock(blockHead, blockTail - 1, BLOCK_SIZE);
+            blockTop = blockTail;
 
-        return block;
+            return block;
+        }
     }
 
     /**
@@ -196,7 +199,7 @@
 
     @Override
     public IdBlock allocateUniqueIdBlock(long range) {
-        throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+        throw new UnsupportedOperationException("Not supported yet");
     }
 
 }
diff --git a/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java b/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
index 230b39b..2f1c4c7 100644
--- a/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
+++ b/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
@@ -23,6 +23,7 @@
 import net.floodlightcontroller.restserver.IRestApiService;
 import net.onrc.onos.core.registry.web.RegistryWebRoutable;
 
+import org.apache.commons.lang.NotImplementedException;
 import org.apache.curator.RetryPolicy;
 import org.apache.curator.framework.CuratorFramework;
 import org.apache.curator.framework.CuratorFrameworkFactory;
@@ -57,46 +58,47 @@
 public class ZookeeperRegistry implements IFloodlightModule, IControllerRegistryService {
 
     private static final Logger log = LoggerFactory.getLogger(ZookeeperRegistry.class);
-    protected String controllerId = null;
 
-    protected IRestApiService restApi;
+    private String controllerId;
+
+    private IRestApiService restApi;
 
     //This is the default, it's overwritten by the connectionString configuration parameter
-    protected String connectionString = "localhost:2181";
+    private String connectionString = "localhost:2181";
 
-    private final String namespace = "onos";
-    private final String switchLatchesPath = "/switches";
+    private static final String NAMESPACE = "onos";
+    private static final String SWITCH_LATCHES_PATH = "/switches";
     private static final String CLUSTER_LEADER_PATH = "/cluster/leader";
 
     private static final String SERVICES_PATH = "/"; //i.e. the root of our namespace
     private static final String CONTROLLER_SERVICE_NAME = "controllers";
 
-    protected CuratorFramework curatorFrameworkClient;
+    private CuratorFramework curatorFrameworkClient;
 
-    protected PathChildrenCache rootSwitchCache;
+    private PathChildrenCache rootSwitchCache;
 
-    protected ConcurrentHashMap<String, SwitchLeadershipData> switches;
-    protected Map<String, PathChildrenCache> switchPathCaches;
+    private ConcurrentHashMap<String, SwitchLeadershipData> switches;
+    private Map<String, PathChildrenCache> switchPathCaches;
 
-    protected LeaderLatch clusterLeaderLatch;
-    protected ClusterLeaderListener clusterLeaderListener;
+    private LeaderLatch clusterLeaderLatch;
+    private ClusterLeaderListener clusterLeaderListener;
     private static final long CLUSTER_LEADER_ELECTION_RETRY_MS = 100;
 
     private static final String ID_COUNTER_PATH = "/flowidcounter";
     private static final Long ID_BLOCK_SIZE = 0x100000000L;
-    protected DistributedAtomicLong distributedIdCounter;
+    private DistributedAtomicLong distributedIdCounter;
 
     //Zookeeper performance-related configuration
-    protected static final int SESSION_TIMEOUT = 5000;
-    protected static final int CONNECTION_TIMEOUT = 7000;
+    private static final int SESSION_TIMEOUT = 7000;    // ms
+    private static final int CONNECTION_TIMEOUT = 5000; // ms
 
     //
     // Unique ID generation state
     // TODO: The implementation must be updated to use the Zookeeper
-    // instead of a ramdon generator.
+    // instead of a random generator.
     //
     private static Random randomGenerator = new Random();
-    private static long nextUniqueIdPrefix = 0;
+    private static long nextUniqueIdPrefix;
     // NOTE: The 0xffffffffL value is used by the Unique ID generator for
     // initialization purpose.
     private static long nextUniqueIdSuffix = 0xffffffffL;
@@ -104,16 +106,33 @@
     private final BlockingQueue<SwitchLeaderEvent> switchLeadershipEvents =
             new LinkedBlockingQueue<SwitchLeaderEvent>();
 
-    private ExecutorService eventThreadExecutorService;
+    /**
+     * Listens for changes to the switch znodes in Zookeeper. This maintains
+     * the second level of PathChildrenCaches that hold the controllers
+     * contending for each switch - there's one for each switch.
+     */
+    private PathChildrenCacheListener switchPathCacheListener =
+            new SwitchPathCacheListener();
+    private ServiceDiscovery<ControllerService> serviceDiscovery;
+    private ServiceCache<ControllerService> serviceCache;
+
 
     private static class SwitchLeaderEvent {
-        public final long dpid;
-        public final boolean isLeader;
+        private final long dpid;
+        private final boolean isLeader;
 
         public SwitchLeaderEvent(long dpid, boolean isLeader) {
             this.dpid = dpid;
             this.isLeader = isLeader;
         }
+
+        public long getDpid() {
+            return dpid;
+        }
+
+        public boolean isLeader() {
+            return isLeader;
+        }
     }
 
     /*
@@ -123,13 +142,13 @@
         while (!Thread.currentThread().isInterrupted()) {
             try {
                 SwitchLeaderEvent event = switchLeadershipEvents.take();
-                SwitchLeadershipData swData = switches.get(HexString.toHexString(event.dpid));
+                SwitchLeadershipData swData = switches.get(HexString.toHexString(event.getDpid()));
                 if (swData == null) {
-                    log.debug("Leadership data {} not found", event.dpid);
+                    log.debug("Leadership data {} not found", event.getDpid());
                     continue;
                 }
 
-                swData.getCallback().controlChanged(event.dpid, event.isLeader);
+                swData.getCallback().controlChanged(event.getDpid(), event.isLeader());
             } catch (InterruptedException e) {
                 Thread.currentThread().interrupt();
                 break;
@@ -139,8 +158,8 @@
         }
     }
 
-    protected class SwitchLeaderListener implements LeaderLatchListener {
-        String dpid;
+    class SwitchLeaderListener implements LeaderLatchListener {
+        private String dpid;
 
         public SwitchLeaderListener(String dpid) {
             this.dpid = dpid;
@@ -161,7 +180,7 @@
         }
     }
 
-    protected class SwitchPathCacheListener implements PathChildrenCacheListener {
+    class SwitchPathCacheListener implements PathChildrenCacheListener {
         @Override
         public void childEvent(CuratorFramework client,
                                PathChildrenCacheEvent event) throws Exception {
@@ -204,10 +223,7 @@
         }
     }
 
-    protected static class ClusterLeaderListener implements LeaderLatchListener {
-        public ClusterLeaderListener() {
-        }
-
+    private static class ClusterLeaderListener implements LeaderLatchListener {
         //
         // NOTE: If we need to support callbacks when the
         // leadership changes, those should be called here.
@@ -224,32 +240,23 @@
         }
     }
 
-    /**
-     * Listens for changes to the switch znodes in Zookeeper. This maintains
-     * the second level of PathChildrenCaches that hold the controllers
-     * contending for each switch - there's one for each switch.
-     */
-    PathChildrenCacheListener switchPathCacheListener = new SwitchPathCacheListener();
-    protected ServiceDiscovery<ControllerService> serviceDiscovery;
-    protected ServiceCache<ControllerService> serviceCache;
-
-
     @Override
     public void requestControl(long dpid, ControlChangeCallback cb) throws RegistryException {
         log.info("Requesting control for {}", HexString.toHexString(dpid));
 
         if (controllerId == null) {
-            throw new RuntimeException("Must register a controller before calling requestControl");
+            throw new IllegalStateException("Must register a controller before calling requestControl");
         }
 
         String dpidStr = HexString.toHexString(dpid);
-        String latchPath = switchLatchesPath + "/" + dpidStr;
 
         if (switches.get(dpidStr) != null) {
             log.debug("Already contesting {}, returning", HexString.toHexString(dpid));
             throw new RegistryException("Already contesting control for " + dpidStr);
         }
 
+        String latchPath = SWITCH_LATCHES_PATH + "/" + dpidStr;
+
         LeaderLatch latch = new LeaderLatch(curatorFrameworkClient, latchPath, controllerId);
         SwitchLeaderListener listener = new SwitchLeaderListener(dpidStr);
         latch.addListener(listener);
@@ -356,17 +363,15 @@
         controllerId = id;
 
         try {
-            ServiceInstance<ControllerService> thisInstance = ServiceInstance.<ControllerService>builder()
+            ServiceInstance<ControllerService> thisInstance =
+                    ServiceInstance.<ControllerService>builder()
                     .name(CONTROLLER_SERVICE_NAME)
                     .payload(new ControllerService(controllerId))
-                            //.port((int)(65535 * Math.random())) // in a real application, you'd use a common port
-                            //.uriSpec(uriSpec)
                     .build();
 
             serviceDiscovery.registerService(thisInstance);
         } catch (Exception e) {
-            // TODO Auto-generated catch block
-            e.printStackTrace();
+            log.error("Exception starting service instance:", e);
         }
 
     }
@@ -389,8 +394,7 @@
             //uses this and that isn't particularly time-sensitive.
             switchCache.rebuild();
         } catch (Exception e) {
-            // TODO Auto-generated catch block
-            e.printStackTrace();
+            log.error("Exception rebuilding the switch cache:", e);
         }
 
         List<ChildData> sortedData = new ArrayList<ChildData>(switchCache.getCurrentData());
@@ -410,7 +414,7 @@
                 }
         );
 
-        if (sortedData.size() == 0) {
+        if (sortedData.isEmpty()) {
             return null;
         }
 
@@ -420,7 +424,7 @@
     @Override
     public Collection<Long> getSwitchesControlledByController(String controller) {
         //TODO remove this if not needed
-        throw new RuntimeException("Not yet implemented");
+        throw new NotImplementedException("Not yet implemented");
     }
 
 
@@ -474,9 +478,11 @@
 
     /**
      * Returns a block of IDs which are unique and unused.
-     * Range of IDs is fixed size and is assigned incrementally as this method called.
-     * Since the range of IDs is managed by Zookeeper in distributed way, this method may block when
-     * requests come up simultaneously.
+     * The range of IDs is a fixed size and is allocated incrementally as this
+     * method is called. Since the range of IDs is managed by Zookeeper in
+     * distributed way, this method may block during Zookeeper access.
+     *
+     * @return an IdBlock containing a set of unique IDs
      */
     @Override
     public IdBlock allocateUniqueIdBlock() {
@@ -563,14 +569,14 @@
                 SESSION_TIMEOUT, CONNECTION_TIMEOUT, retryPolicy);
 
         curatorFrameworkClient.start();
-        curatorFrameworkClient = curatorFrameworkClient.usingNamespace(namespace);
+        curatorFrameworkClient = curatorFrameworkClient.usingNamespace(NAMESPACE);
 
         distributedIdCounter = new DistributedAtomicLong(
                 curatorFrameworkClient,
                 ID_COUNTER_PATH,
                 new RetryOneTime(100));
 
-        rootSwitchCache = new PathChildrenCache(curatorFrameworkClient, switchLatchesPath, true);
+        rootSwitchCache = new PathChildrenCache(curatorFrameworkClient, SWITCH_LATCHES_PATH, true);
         rootSwitchCache.getListenable().addListener(switchPathCacheListener);
 
         //Build the service discovery object
@@ -590,11 +596,12 @@
             //Don't prime the cache, we want a notification for each child node in the path
             rootSwitchCache.start(StartMode.NORMAL);
         } catch (Exception e) {
-            throw new FloodlightModuleException("Error initialising ZookeeperRegistry: "
-                    + e.getMessage());
+            throw new FloodlightModuleException(
+                    "Error initialising ZookeeperRegistry", e);
         }
 
-        eventThreadExecutorService = Executors.newSingleThreadExecutor();
+        ExecutorService eventThreadExecutorService =
+                Executors.newSingleThreadExecutor();
         eventThreadExecutorService.execute(
                 new Runnable() {
                     @Override