Cleanup TODOs and FIXMEs

- Fix minor ones
- Report bigger and include issue number in comment
- Pull common dependencies into dependency management of common/parents

Change-Id: I06a6ac37c52b603fd73ed42023d6b2e7fa18010f
Signed-off-by: Maros Marsalek <mmarsale@cisco.com>

9 files changed, 31 insertions, 57 deletions

diff --git a/infra/translate-utils/pom.xml b/infra/translate-utils/pom.xml
index 51b1194f1..f2b5a0c75 100644
--- a/infra/translate-utils/pom.xml
+++ b/infra/translate-utils/pom.xml
@@ -28,24 +28,9 @@
     <version>1.16.12-SNAPSHOT</version>
     <packaging>bundle</packaging>
 
-    <dependencyManagement>
-        <dependencies>
-            <dependency>
-                <groupId>org.opendaylight.mdsal</groupId>
-                <artifactId>mdsal-artifacts</artifactId>
-                <version>2.0.2-Beryllium-SR2</version>
-                <type>pom</type>
-                <scope>import</scope>
-            </dependency>
-            <dependency>
-                <groupId>org.opendaylight.controller</groupId>
-                <artifactId>mdsal-artifacts</artifactId>
-                <version>1.3.2-Beryllium-SR2</version>
-                <type>pom</type>
-                <scope>import</scope>
-            </dependency>
-        </dependencies>
-    </dependencyManagement>
+    <properties>
+        <jgrapht.version>0.9.2</jgrapht.version>
+    </properties>
 
     <dependencies>
         <dependency>
@@ -73,10 +58,9 @@
         <dependency>
             <groupId>org.jgrapht</groupId>
             <artifactId>jgrapht-core</artifactId>
-            <version>0.9.2</version>
+            <version>${jgrapht.version}</version>
         </dependency>
 
-        <!-- Testing Dependencies -->
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
@@ -87,7 +71,5 @@
             <artifactId>mockito-all</artifactId>
             <scope>test</scope>
         </dependency>
-
     </dependencies>
-
 </project>
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/AbstractSubtreeManagerRegistryBuilderBuilder.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/AbstractSubtreeManagerRegistryBuilderBuilder.java
index bf1e89c12..07f2bf1ee 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/AbstractSubtreeManagerRegistryBuilderBuilder.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/AbstractSubtreeManagerRegistryBuilderBuilder.java
@@ -192,10 +192,10 @@ public abstract class AbstractSubtreeManagerRegistryBuilderBuilder<S extends Sub
                     }
                 });
 
-        // TODO we could optimize subtree handlers, if there is a dedicated handler for a node managed by a subtree
+        // TODO HONEYCOMB-171 we could optimize subtree handlers, if there is a dedicated handler for a node managed by a subtree
         // handler, recreate the subtree handler with a subset of handled child nodes
         // This way it is not necessary to change the configuration of subtree writer, just to add a dedicated child
-        // writer. This will be needed if we ever switch to annotations for reader/writer hierarchy initialization
+        // writer
 
         return builder.build();
     }
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/RWUtils.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/RWUtils.java
index 695ce6806..4616f8347 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/RWUtils.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/RWUtils.java
@@ -38,10 +38,12 @@ import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 
 public final class RWUtils {
 
+    // TODO HONEYCOMB-172 update the utils methods considering Java8. Make sure they still work by wiring a detailed unit test first
+
     private RWUtils() {}
 
     /**
-     * Collector expecting only a single resulting item from a stream
+     * Collector expecting only a single resulting item from a stream.
      */
     public static<T> Collector<T,?,T> singleItemCollector() {
         return Collectors.collectingAndThen(
@@ -56,12 +58,11 @@ public final class RWUtils {
     }
 
     /**
-     * Find next item in ID after provided type
+     * Find next item in ID after provided type.
      */
     @Nonnull
     public static InstanceIdentifier.PathArgument getNextId(@Nonnull final InstanceIdentifier<? extends DataObject> id,
                                                             @Nonnull final InstanceIdentifier<? extends DataObject> type) {
-        // TODO this is inefficient(maybe, depending on actual Iterable type)
         final Iterable<InstanceIdentifier.PathArgument> pathArguments = id.getPathArguments();
         final int i = Iterables.indexOf(pathArguments, new Predicate<InstanceIdentifier.PathArgument>() {
             @Override
@@ -74,7 +75,7 @@ public final class RWUtils {
     }
 
     /**
-     * Replace last item in ID with a provided IdentifiableItem of the same type
+     * Replace last item in ID with a provided IdentifiableItem of the same type.
      */
     @SuppressWarnings("unchecked")
     @Nonnull
@@ -90,7 +91,7 @@ public final class RWUtils {
     }
 
     /**
-     * Create IdentifiableItem from target type of provided ID with provided key
+     * Create IdentifiableItem from target type of provided ID with provided key.
      */
     @Nonnull
     public static <D extends DataObject & Identifiable<K>, K extends Identifier<D>> InstanceIdentifier.IdentifiableItem<D, K> getCurrentIdItem(
@@ -99,7 +100,7 @@ public final class RWUtils {
     }
 
     /**
-     * Trim InstanceIdentifier at indexOf(type)
+     * Trim InstanceIdentifier at indexOf(type).
      */
     @SuppressWarnings("unchecked")
     @Nonnull
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/TransactionMappingContext.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/TransactionMappingContext.java
index 1b6504c78..4d4e9fddd 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/TransactionMappingContext.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/TransactionMappingContext.java
@@ -34,7 +34,7 @@ public class TransactionMappingContext implements MappingContext {
 
     private final ReadWriteTransaction readWriteTransaction;
 
-    // TODO make async
+    // TODO HONEYCOMB-169 make async
 
     public TransactionMappingContext(final ReadWriteTransaction readWriteTransaction) {
         this.readWriteTransaction = readWriteTransaction;
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/AbstractGenericReader.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/AbstractGenericReader.java
index 75a2a673c..40c78b3c9 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/AbstractGenericReader.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/AbstractGenericReader.java
@@ -56,7 +56,8 @@ public abstract class AbstractGenericReader<D extends DataObject, B extends Buil
                                       @Nonnull final ReadContext ctx) throws ReadFailedException {
         LOG.debug("{}: Reading current: {}", this, id);
         final B builder = getBuilder(id);
-        // Cache empty value to determine if anything has changed later TODO cache in a field
+        // The empty value could be cached, but no caching is safer since we call overridden getBuilder each time
+        // and the build could produce something different (even if it shouldn't)
         final D emptyValue = builder.build();
 
         LOG.trace("{}: Reading current attributes", this);
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/registry/SubtreeReader.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/registry/SubtreeReader.java
index 720bd0b11..260fb241b 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/registry/SubtreeReader.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/read/registry/SubtreeReader.java
@@ -19,7 +19,6 @@ package io.fd.honeycomb.translate.util.read.registry;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.Optional;
-import com.google.common.base.Predicate;
 import com.google.common.collect.Iterables;
 import io.fd.honeycomb.translate.read.ListReader;
 import io.fd.honeycomb.translate.read.ReadContext;
@@ -34,7 +33,6 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 import org.opendaylight.yangtools.concepts.Builder;
 import org.opendaylight.yangtools.yang.binding.DataObject;
 import org.opendaylight.yangtools.yang.binding.Identifiable;
@@ -143,7 +141,6 @@ class SubtreeReader<D extends DataObject, B extends Builder<D>> implements Reade
     private static Optional<? extends DataObject> findNextParent(@Nonnull final DataObject parent,
                                                                  @Nonnull final InstanceIdentifier.PathArgument nextId,
                                                                  @Nonnull final Class<?> managedType) {
-        // TODO is there a better way than reflection ? e.g. convert into NN and filter out with a utility
         Optional<Method> method = ReflectionUtils.findMethodReflex(managedType, "get",
                 Collections.emptyList(), nextId.getType());
 
@@ -172,19 +169,13 @@ class SubtreeReader<D extends DataObject, B extends Builder<D>> implements Reade
         checkArgument(nextId instanceof InstanceIdentifier.IdentifiableItem<?, ?>,
                 "Unable to perform wildcarded read for %s", nextId);
         final Identifier key = ((InstanceIdentifier.IdentifiableItem) nextId).getKey();
-        // TODO replace with stream().filter().findFirst() when we switch to using java's Optional instead of Guava's
-        // because now we would have to do awkward Optional transformation since findFirstReturns guava's optional
-        return Iterables.tryFind(invoke, new Predicate<DataObject>() {
-
-            @Override
-            public boolean apply(@Nullable final DataObject input) {
-                final Optional<Method> keyGetter = ReflectionUtils.findMethodReflex(nextId.getType(), "get",
-                                Collections.emptyList(), key.getClass());
-                final Object actualKey;
-                actualKey = invoke(keyGetter.get(), nextId, input);
-                return key.equals(actualKey);
-            }
-        });
+
+        final Method keyGetter = ReflectionUtils.findMethodReflex(nextId.getType(), "get",
+                        Collections.emptyList(), key.getClass()).get();
+
+        return Optional.fromNullable(invoke.stream()
+                .filter(item -> key.equals(invoke(keyGetter, nextId, item)))
+                .findFirst().orElse(null));
     }
 
     private static DataObject filterSingle(final DataObject parent,
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
index 5d801edab..0128ee4a8 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/TransactionWriteContext.java
@@ -50,14 +50,13 @@ public final class TransactionWriteContext implements WriteContext {
                                    final DOMDataReadOnlyTransaction afterTx,
                                    final MappingContext mappingContext) {
         this.serializer = serializer;
-        // TODO do we have a BA transaction adapter ? If so, use it here and don't pass serializer
         this.beforeTx = beforeTx;
         this.afterTx = afterTx;
         this.mappingContext = mappingContext;
         this.ctx = new ModificationCache();
     }
 
-    // TODO make this asynchronous
+    // TODO HONEYCOMB-169 make this asynchronous
 
     @Override
     public <T extends DataObject> Optional<T> readBefore(@Nonnull final InstanceIdentifier<T> currentId) {
@@ -78,7 +77,7 @@ public final class TransactionWriteContext implements WriteContext {
                 tx.read(LogicalDatastoreType.CONFIGURATION, path);
 
         try {
-            // TODO once the APIs are asynchronous use just Futures.transform
+            // TODO HONEYCOMB-169 once the APIs are asynchronous use just Futures.transform
             final Optional<NormalizedNode<?, ?>> optional = read.checkedGet();
 
             if (!optional.isPresent()) {
diff --git a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
index df8ec107b..ab80eb4ac 100644
--- a/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
+++ b/infra/translate-utils/src/main/java/io/fd/honeycomb/translate/util/write/registry/FlatWriterRegistry.java
@@ -139,13 +139,11 @@ final class FlatWriterRegistry implements WriterRegistry {
     }
 
     private Writer<?> getSubtreeWriterResponsible(final InstanceIdentifier<?> singleType) {
-        final Writer<?> writer;// This is slow ( minor TODO-perf )
-        writer = writers.values().stream()
+        return writers.values().stream()
                 .filter(w -> w instanceof SubtreeWriter)
                 .filter(w -> ((SubtreeWriter<?>) w).getHandledChildTypes().contains(singleType))
                 .findFirst()
                 .get();
-        return writer;
     }
 
     private Collection<DataObjectUpdate> getParentDataObjectUpdate(final WriteContext ctx,
@@ -255,7 +253,6 @@ final class FlatWriterRegistry implements WriterRegistry {
         return writers.get(singleType);
     }
 
-    // FIXME unit test
     private final class ReverterImpl implements Reverter {
 
         private final Collection<InstanceIdentifier<?>> processedNodes;
diff --git a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/impl/write/util/TransactionWriteContextTest.java b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/impl/write/util/TransactionWriteContextTest.java
index 001e5567a..79155bdd6 100644
--- a/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/impl/write/util/TransactionWriteContextTest.java
+++ b/infra/translate-utils/src/test/java/io/fd/honeycomb/translate/impl/write/util/TransactionWriteContextTest.java
@@ -29,10 +29,10 @@ import static org.mockito.MockitoAnnotations.initMocks;
 
 import com.google.common.base.Optional;
 import com.google.common.util.concurrent.Futures;
+import io.fd.honeycomb.translate.MappingContext;
 import io.fd.honeycomb.translate.ModificationCache;
 import io.fd.honeycomb.translate.util.DataObjects;
 import io.fd.honeycomb.translate.util.write.TransactionWriteContext;
-import io.fd.honeycomb.translate.MappingContext;
 import java.util.Map;
 import org.junit.Before;
 import org.junit.Test;
@@ -125,7 +125,10 @@ public class TransactionWriteContextTest {
     @Test
     public void testClose() throws Exception {
         final ModificationCache context = transactionWriteContext.getModificationCache();
+        final Object o = new Object();
+        context.put(o, o);
+        assertTrue(context.containsKey(o));
         transactionWriteContext.close();
-        // TODO verify context was closed
+        assertFalse(context.containsKey(o));
     }
 }
\ No newline at end of file