trjhtr

My app is about deleting exif metadata from pictures. Also, what do you think about the code quality (OOP principles, clean code)? Is it production-ready?

GitHub

MetaWipe.java

public final class MetaWipe 

private final ArgumentResolver resolver = new ArgumentResolver();
private final ExifEraserService eraser = new ExifEraserServiceImpl();
private CommandLine params = null;

private static final char FILE_ARG = 'f';
private static final char DIR_ARG = 'd';
private static final String HELP_ARG = "help";

private static final String VERSION = "0.1";

private void run(final String args) 
 parseCommandLineArguments(args);
 if(params != null) 
 if (params.hasOption(FILE_ARG) && (!params.hasOption(DIR_ARG) && !params.hasOption(HELP_ARG))) 
 tryEraseExifOrExitOnFailure(params.getOptionValue(FILE_ARG));
 else if (params.hasOption(DIR_ARG) && !params.hasOption(FILE_ARG)) 
 tryEraseExifInDir(params.getOptionValue(DIR_ARG));
 else if(params.hasOption(HELP_ARG) && (!params.hasOption(FILE_ARG) && !params.hasOption(DIR_ARG))) 
 resolver.displayUsage();
 else 
 displayInfo();
 
 else resolver.displayUsage();


private void tryEraseExifInDir(final String pathValue) 
 if (!empty(pathValue)) 
 try 
 final Path dir = Paths.get(pathValue);
 eraser.directory(dir);
 catch (NotDirectoryException nde) 
 exitWithErrorMessage("This is not a directory: " + pathValue, 2);
 catch (FileNotFoundException fne) 
 exitWithErrorMessage("Directory not found : " + pathValue, 2);
 catch (IOException e) 
 exitWithErrorMessage("An I/O error occurred: " + e.getMessage(), 2);
 
 else 
 exitWithErrorMessage("You must supply a path", 1);
 


private void tryEraseExifOrExitOnFailure(final String pathValue) 
 if (!empty(pathValue)) 
 try 
 final Path path = Paths.get(pathValue);
 eraser.file(path);
 catch (NotAFileException nfe) 
 exitWithErrorMessage("This is not a file! " + pathValue, 2);
 catch (FileNotFoundException e) 
 exitWithErrorMessage("File not found! " + pathValue, 2);
 catch (ImageReadException ire) 
 exitWithErrorMessage("Can't read image! " + ire.getMessage(), 4);
 catch (ImageWriteException iwe) 
 exitWithErrorMessage("Can't write image! " + iwe.getMessage(), 4);
 catch (IOException e) 
 exitWithErrorMessage("An I/O error occurred: " + e.getMessage(), 2);
 
 else 
 exitWithErrorMessage("You must supply a path !", 1);
 


private void parseCommandLineArguments(String args) 
 try 
 params = resolver.parseCommandLineArguments(args);
 catch (ParseException e) 
 exitWithErrorMessage("Invalid parameters !", 1);
 


private boolean empty(String str) 

private void exitWithErrorMessage(String msg, int exitCode) 
 System.out.println(msg);
 resolver.displayUsage();
 System.exit(exitCode);


private void displayInfo() 
 System.out.println("metawipe (Version: " + VERSION +")");
 System.out.println("Small command-line tool to clear the metadata/exif records of your photos to give back the control over security and privacy.");
 System.out.println("It is useful to clear metadata before uploading your photos to cloud like Facebook, Google, etc if you don't want to share your GPS location");
 System.out.println("and other sensitive data with anyone.");
 System.out.println("Type <metawipe -help> to display the help.");
 System.out.println("Visit https://github.com/kivimango/metawipe for more info.");


public static void main(String args) 
 MetaWipe app = new MetaWipe();
 app.run(args);

ArgumentResolver.java

final class ArgumentResolver 

private static final String FILE_FLAG = "f";
private static final String DIR_FLAG = "d";
private static final String HELP_FLAG = "help";

private final Options flagOptions = new Options();
private final CommandLineParser parser = new DefaultParser();

ArgumentResolver() 
 makeOptions();


private void makeOptions() 
 Option fileOption = Option.builder(FILE_FLAG).optionalArg(true).argName("file").hasArg().desc("Clears metadata of the given photo").build();
 Option dirOption = Option.builder(DIR_FLAG).optionalArg(true).argName("dir").hasArg().desc("Clears every photo in a directory and its subdirectories recursively").build();
 Option helpOption = Option.builder(HELP_FLAG).optionalArg(true).argName("help").hasArg(false).desc("Displays this help").build();
 flagOptions.addOption(fileOption);
 flagOptions.addOption(dirOption);
 flagOptions.addOption(helpOption);


final CommandLine parseCommandLineArguments(String args) throws ParseException 
 return parser.parse(flagOptions, args);


final void displayUsage() 
 final HelpFormatter formatter = new HelpFormatter();
 formatter.printHelp( "metawipe", flagOptions);

ExifEraserServiceImpl.java

public final class ExifEraserServiceImpl implements ExifEraserService 

private final ExifRewriter rewriter = new ExifRewriter();
private final List<String> supportedFormats = Arrays.asList("jpg", "jpeg", "tiff");

@Override
public final void directory(final Path dir) throws IOException 
 if(!Files.exists(dir)) throw new FileNotFoundException();
 if(!Files.isDirectory(dir)) 
 throw new NotDirectoryException(dir.toString());
 else 
 Files.walkFileTree(dir, new RecursiveDirectoryWalker(this));
 


/**
 * Before deleting exif data of the image, we have to make a copy of it.
 * Otherwise Imaging library will throw an EOF exception if we want to read and write to the same file.
 * After successful deletion, the copy gets renamed to the original file, and the original file will be overridden.
 */

@Override
public final boolean file(final Path file) throws IOException, ImageWriteException, ImageReadException, NotAFileException 
 if(!Files.exists(file)) throw new FileNotFoundException();
 if(Files.isDirectory(file)) throw new NotAFileException();
 if(supportedFormats.contains(FileNameResolver.getExtension(file))) 
 final Path copiedFile = makeCopy(file);
 deleteExifMetaData(file, copiedFile);
 Files.move(copiedFile, file, REPLACE_EXISTING);
 
 return checkExifDeleted(file);


private void deleteExifMetaData(final Path f, final Path copy) throws IOException, ImageWriteException, ImageReadException 
 try (FileOutputStream fos = new FileOutputStream(copy.toFile()); OutputStream os = new BufferedOutputStream(fos)) 
 try
 rewriter.removeExifMetadata(f.toFile(), os);
 /* During deleting, exceptions may occur.
 In this case, we have to delete the copy of the original file */
 catch (ImageReadException e) 
 Files.deleteIfExists(copy);
 throw new ImageReadException(e.getMessage(), e);
 catch (ImageWriteException e) 
 Files.deleteIfExists(copy);
 throw new ImageWriteException(e.getMessage(), e);
 
 


Path makeCopy(final Path original) throws IOException 
 final String tempFileName = FileNameResolver.getFilePath(original) + File.separator +
 FileNameResolver.getFileNameWithoutExtension(original) + "_copy." + FileNameResolver.getExtension(original);
 final Path copiedFilePath = Paths.get(tempFileName);
 if(Files.exists(copiedFilePath)) Files.deleteIfExists(copiedFilePath); 
 Files.copy(original, copiedFilePath);
 return copiedFilePath;


boolean checkExifDeleted(final Path f) throws IOException, ImageReadException 
 // Sometimes metadata is null even if the exif records deleted
 final IImageMetadata metadata = Imaging.getMetadata(f.toFile());
 return metadata == null 

RecursiveDirectoryWalker.java

class RecursiveDirectoryWalker implements FileVisitor<Path> 

private final PathMatcher matcher = FileSystems.getDefault().getPathMatcher("glob:**.jpg,jpeg,tiff");
private ExifEraserService eraser;

RecursiveDirectoryWalker(ExifEraserService eraser) 
 this.eraser = eraser;


@Override
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException 
 Stream<Path> list = Files.list(dir);
 if (list != null) 
 list.filter(matcher::matches)
 .forEach((Path f) -> IOException ie) 
 System.out.println("Error: " + ie);
 catch (NotAFileException ignore) 
 
 );
 
 return FileVisitResult.CONTINUE;


@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException 
 if(matcher.matches(file)) 
 try 
 eraser.file(file);
 catch (ImageWriteException 
 return FileVisitResult.CONTINUE;


@Override
public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException 
 if(exc != null) 
 System.out.println("Error: " + exc);
 
 return FileVisitResult.CONTINUE;


@Override
public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException 
 if(exc != null) 
 System.out.println("Error: " + exc);
 
 return FileVisitResult.CONTINUE;

FileNameResolver.java

final class FileNameResolver 

static String getFilePath(final Path file) 
 String absolutePath = file.toAbsolutePath().toString();
 return absolutePath.substring(0,absolutePath.lastIndexOf(File.separator));


// TODO : duplicate code

static String getFileNameWithoutExtension(final Path file) 
 String fileName = file.getFileName().toString();
 int pos = fileName.lastIndexOf(".");
 if (pos > 0) 
 fileName = fileName.substring(0, pos);
 
 return fileName;


static String getExtension(final Path file) 
 String fileName = file.getFileName().toString();
 int pos = fileName.lastIndexOf(".");
 if (pos > 0) 
 fileName = fileName.substring(pos + 1);
 
 return fileName;

• The formatting is inconsistent, perhaps let the IDE do it if you're
  using one.


• It's a bit verbose, that goes both for the classes present here as
  well as the test code in the repository. If a variable is only
  defined and used two times I really don't see the point of having it,
  outside perhaps of the minor argument that it helps in not-so-great
  debuggers.


• Same goes for the constants in the ArgumentResolver class. Other
  than that the class looks okay, having a dedicated piece to parse
  command line options is good; again displayUsage could just be
  new HelpFormatter().printHelp("metawipe", flagOptions); and you'd
  already have eliminated about four tokens to mentally process.


• I'd guess that PNG files can have EXIF metadata? What about other
  file formats that the used library supports?


• Use better method names - file, directory are at best variable
  names, but don't say anything about what the method does. In this
  case (ExifEraserService) I'd consider visitFile, processFile, or
  even deleteExifFromFile or so on.


• The general pattern of copying the file, editing it, then moving it
  over the old one is good. I don't get why checkExifDeleted is
  necessary but I guess there are technicalities involved? The check
  via toString().contains(...) sounds like a hack though, would that
  still work if the metadata itself contains that string (as a title or
  comment or whatever)?


• 
  makeCopy is dangerous (what if the user has an actual file with the
  "temporary" name?). Consider using java.io.Files.createTempFile or
  even java.nio.Files.createTempDirectory instead.


• The catchs in deleteExifMetaData can be a single case I think, as
  you've already done in preVisitDirectory.


• 
  FileNameResolver has duplicate code as you've already written ...


• Finally MetaWipe has the constants duplicated for some reason? The
  handlers in run look weird, just put them in the order they should
  be handled and exit early - if -h is already specified, why do you
  care if some other options is set as well?


• The exception handling is again a bit verbose, I understand that you
  want to provide nice error messages, but there's again a lot of
  duplication here and I'd drop the different exit statuses, do you
  really check those?


• Also for both try... methods, maybe invert the check:
  if (empty(pathValue)) exit...; ... then you can eliminate one level
  of nesting already.


• Just a general note: All the error checking is nice, but there are
  still situations where a file or directory might be deleted, modified,
  etc. while you're iterating through the file tree. Like in the
  directory method - the directory could have been deleted after the
  isDirectory check, but before Files.walkFileTree would have
  started to work ... so my pragmatic suggestion would actually be to
  just log the IOExceptions and add a bit of context (current file
  visited) with a wrapped exception or so.

Now that I went through all of this I'd say it looks okay, can be
cleaned up a bit like I wrote above. Also maybe look at how other
command line programs behave, usually they don't even have a -f or
-d flag to switch between the two behaviours you have here and instead
accept an arbitrary number of file/directory arguments.