1
\$\begingroup\$

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) {
    return str == null || str.trim().isEmpty();
}

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 || metadata.toString().contains("No Exif metadata.");
}

}

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) -> {
                    try {
                        eraser.file(f);
                    } catch (ImageWriteException | ImageReadException | 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 | ImageReadException ie) {
            System.out.println("Error: " + ie);
        } catch (NotAFileException ignore) {
        }
    }
    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;
}

}
\$\endgroup\$
0
3
\$\begingroup\$
  • 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.