Fix code smells (#175)

* Fix smells * added test * remove redundant close calls
osmlab · Jul 17, 2018 · 0f108ce · 0f108ce
1 parent b45a401
commit 0f108ce
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 45 deletions.
diff --git a/src/main/java/org/openstreetmap/atlas/geography/PolyLine.java b/src/main/java/org/openstreetmap/atlas/geography/PolyLine.java
@@ -81,9 +81,10 @@ public static PolyLine random(final int numberPoints, final Rectangle bounds)
     public static void saveAsGeoJson(final Iterable<? extends Iterable<Location>> geometries,
             final WritableResource resource)
     {
-        final JsonWriter writer = new JsonWriter(resource);
-        writer.write(asGeoJson(geometries).jsonObject());
-        writer.close();
+        try (final JsonWriter writer = new JsonWriter(resource))
+        {
+            writer.write(asGeoJson(geometries).jsonObject());
+        }
     }
 
     /**

diff --git a/src/main/java/org/openstreetmap/atlas/geography/sharding/preparation/TilePrinter.java b/src/main/java/org/openstreetmap/atlas/geography/sharding/preparation/TilePrinter.java
@@ -26,7 +26,7 @@ public class TilePrinter extends Command
 
     private static final Switch<File> OUTPUT_FOLDER = new Switch<>("output", "The output folder",
             value -> new File(value));
-    private static final Switch<Integer> ZOOM = new Switch<>("zoom", "The zoom",
+    private static final Switch<Integer> ZOOM_SWITCH = new Switch<>("zoom", "The zoom",
             value -> Integer.valueOf(value));
 
     private int index;
@@ -42,7 +42,7 @@ public static void main(final String[] args)
     protected int onRun(final CommandMap command)
     {
         this.index = 0;
-        this.zoom = (int) command.get(ZOOM);
+        this.zoom = (int) command.get(ZOOM_SWITCH);
         this.folder = (File) command.get(OUTPUT_FOLDER);
         this.folder.mkdirs();
         SafeBufferedWriter writer = getNextFile().writer();
@@ -111,7 +111,7 @@ protected int onRun(final CommandMap command)
     @Override
     protected SwitchList switches()
     {
-        return new SwitchList().with(ZOOM, OUTPUT_FOLDER);
+        return new SwitchList().with(ZOOM_SWITCH, OUTPUT_FOLDER);
     }
 
     private File getNextFile()

diff --git a/src/main/java/org/openstreetmap/atlas/streaming/resource/zip/ZipFileWritableResource.java b/src/main/java/org/openstreetmap/atlas/streaming/resource/zip/ZipFileWritableResource.java
@@ -28,10 +28,8 @@ public ZipFileWritableResource(final File source)
     @Override
     public Iterable<Resource> entries()
     {
-        ZipFile file = null;
-        try
+        try (final ZipFile file = new ZipFile(getFileSource().getFile()))
         {
-            file = new ZipFile(getFileSource().getFile());
             final ZipFile fileCopy = file;
             return Iterables.translate(Iterables.from(file.entries()), entry ->
             {
@@ -50,7 +48,6 @@ public Iterable<Resource> entries()
         }
         catch (final IOException e)
         {
-            Streams.close(file);
             throw new CoreException("Cannot get entries from the Zipfile {}.",
                     this.getFileSource().getName(), e);
         }
@@ -66,10 +63,8 @@ public Iterable<Resource> entries()
      */
     public Resource entryForName(final String name)
     {
-        ZipFile file = null;
-        try
+        try (final ZipFile file = new ZipFile(getFileSource().getFile()))
         {
-            file = new ZipFile(getFileSource().getFile());
             final ZipEntry entry = file.getEntry(name);
             if (entry != null)
             {
@@ -84,7 +79,6 @@ public Resource entryForName(final String name)
         }
         catch (final IOException e)
         {
-            Streams.close(file);
             throw new CoreException("Cannot get the entry {} from the Zipfile {}.", name,
                     this.getFileSource().getName(), e);
         }

diff --git a/src/main/java/org/openstreetmap/atlas/utilities/archive/Extractor.java b/src/main/java/org/openstreetmap/atlas/utilities/archive/Extractor.java
@@ -12,7 +12,6 @@
 import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
 import org.apache.commons.compress.archivers.zip.ZipFile;
 import org.apache.commons.io.FileUtils;
-import org.apache.commons.io.IOUtils;
 import org.openstreetmap.atlas.streaming.NotifyingIOUtils;
 import org.openstreetmap.atlas.streaming.NotifyingIOUtils.IOProgressListener;
 
@@ -182,44 +181,46 @@ public Extractor extract(final File inputFile) throws ArchiveException, IOExcept
         }
 
         fireArchiveStarted();
-        final ZipFile file = new ZipFile(inputFile);
-        for (final ZipArchiveEntry current : Collections.list(file.getEntries()))
+        try (final ZipFile file = new ZipFile(inputFile))
         {
-            final File outputFile = new File(this.outputDirectory, current.getName());
-
-            if (shouldSkip(outputFile))
-            {
-                fireItemSkipped(outputFile);
-            }
-            else if (outputFile.exists() && this.skipExisting)
+            for (final ZipArchiveEntry current : Collections.list(file.getEntries()))
             {
-                fireItemSkipped(outputFile);
+                final File outputFile = new File(this.outputDirectory, current.getName());
+
+                if (shouldSkip(outputFile))
+                {
+                    fireItemSkipped(outputFile);
+                }
+                else if (outputFile.exists() && this.skipExisting)
+                {
+                    fireItemSkipped(outputFile);
+                }
+                else if (current.isDirectory())
+                {
+                    continue;
+                }
+                else
+                {
+                    try (final BufferedOutputStream bos = new BufferedOutputStream(
+                            new FileOutputStream(outputFile));
+                            final InputStream inputStream = file.getInputStream(current))
+                    {
+                        outputFile.getParentFile().mkdirs();
+                        NotifyingIOUtils.copy(inputStream, bos,
+                                new Progress(outputFile, current.getSize()));
+                    }
+                }
+
             }
-            else if (current.isDirectory())
+            if (this.errorCount > 0)
             {
-                continue;
+                fireArchiveFailed();
             }
             else
             {
-                outputFile.getParentFile().mkdirs();
-                final BufferedOutputStream bos = new BufferedOutputStream(
-                        new FileOutputStream(outputFile));
-                final InputStream inputStream = file.getInputStream(current);
-                NotifyingIOUtils.copy(inputStream, bos,
-                        new Progress(outputFile, current.getSize()));
-                IOUtils.closeQuietly(inputStream);
-                IOUtils.closeQuietly(bos);
+                fireArchiveCompleted();
             }
         }
-        if (this.errorCount > 0)
-        {
-            fireArchiveFailed();
-        }
-        else
-        {
-            fireArchiveCompleted();
-        }
-        file.close();
         return this;
     }
 

diff --git a/src/main/java/org/openstreetmap/atlas/utilities/collections/Iterables.java b/src/main/java/org/openstreetmap/atlas/utilities/collections/Iterables.java
@@ -291,7 +291,7 @@ public static <Type> boolean equals(final Iterable<Type> that, final Iterable<Ty
         final boolean otherIsNull = other == null;
         if (thatIsNull || otherIsNull)
         {
-            return thatIsNull & thatIsNull;
+            return thatIsNull && otherIsNull;
         }
 
         // Iterables are not null, let's check for size first

diff --git a/src/test/java/org/openstreetmap/atlas/utilities/archive/ExtractorTest.java b/src/test/java/org/openstreetmap/atlas/utilities/archive/ExtractorTest.java
@@ -0,0 +1,48 @@
+package org.openstreetmap.atlas.utilities.archive;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.commons.compress.archivers.ArchiveException;
+import org.junit.Assert;
+import org.junit.Test;
+import org.openstreetmap.atlas.streaming.resource.InputStreamResource;
+import org.openstreetmap.atlas.streaming.resource.Resource;
+
+/**
+ * @author matthieun
+ */
+public class ExtractorTest
+{
+    @Test
+    public void testExtract() throws IOException, ArchiveException
+    {
+        File temporaryFile = null;
+        File outputDirectory = null;
+        try
+        {
+            final String name = "testExtractor";
+            temporaryFile = File.createTempFile(name, ".zip");
+            outputDirectory = File.createTempFile(name + "_output", "");
+            outputDirectory.delete();
+            outputDirectory.mkdirs();
+            final Resource testData = new InputStreamResource(
+                    () -> ExtractorTest.class.getResourceAsStream("testExtractor.zip"));
+            testData.copyTo(new org.openstreetmap.atlas.streaming.resource.File(temporaryFile));
+            final Extractor extractor = Extractor.extractZipArchive(outputDirectory);
+            extractor.extract(temporaryFile);
+            final String expectedOutput = "file1";
+            final String actualOutput = new org.openstreetmap.atlas.streaming.resource.File(
+                    new File(outputDirectory, "file1.txt")).all();
+            Assert.assertEquals(expectedOutput, actualOutput);
+        }
+        finally
+        {
+            temporaryFile.delete();
+            new org.openstreetmap.atlas.streaming.resource.File(outputDirectory)
+                    .listFilesRecursively()
+                    .forEach(org.openstreetmap.atlas.streaming.resource.File::delete);
+            outputDirectory.delete();
+        }
+    }
+}
diff --git a/src/test/resources/org/openstreetmap/atlas/utilities/archive/testExtractor.zip b/src/test/resources/org/openstreetmap/atlas/utilities/archive/testExtractor.zip