3
\$\begingroup\$

I have a implementation of following code block,

public class FileFormatConversion {

public static enum File_TYPES {

  PDF, PNG, OTHERS, ALL
}

public static void processOtherFileType(String arg1) {
    System.out.println("Other file output -- " + arg1 + " file");
}

public static void processPNGPDFALL(String arg1, String arg2) {
    System.out
            .println("General block for processing PDF/PNG or ALL other file format -- "
                    + arg2
                    + "\\"
                    + arg1
                    + " parameter validation/check file availability");
}

public static void processPDF(String arg1, String arg2) {
    System.out.println("Block for processing PDF file -- " + arg2 + "\\"
            + arg1 + ".pdf");
}

public static void processALL(String arg1, String arg2) {
    System.out.println("Block for processing ALL file types -- " + arg2
            + "\\" + arg1 + ".all");
}

public static void processPNGALL(String arg1, String arg2) {
    System.out
            .println("Common block for manipulating file formats such as PNG or ALL files");
}

/**
 * @param args
 */
public static void main(String[] args) {

    args = new String[3];

    args[0] = "ALL";
    args[1] = "fileName";
    args[2] = "C:\\filePath";

    File_TYPES fileFormatInput = File_TYPES.valueOf(args[0]);

    if (File_TYPES.OTHERS == fileFormatInput) {
        processOtherFileType(args[1]);
    } else {
        processPNGPDFALL(args[1], args[2]);
        if (File_TYPES.ALL == fileFormatInput
                || File_TYPES.PNG == fileFormatInput)
            processPNGALL(args[1], args[2]);
        if (File_TYPES.ALL == fileFormatInput)
            processALL(args[1], args[2]);
        if (File_TYPES.PDF == fileFormatInput)
            processPDF(args[1], args[2]);
    }

    System.out.println("Completed");
}

I might need to add more options in ENUM in future. so using if-else will make it complex while debugging/adding. Is there any better alternative way to improve this block - improve understandability/maintenance.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please provide a more concrete use case, with real working code, not pseudocode, so that we can better understand your problem. \$\endgroup\$ – 200_success Oct 4 '13 at 5:33
  • \$\begingroup\$ @200_success: i have updated the code block. though it is not exact snippet, i have added all required methods (with exact input/output parameters) in class to replicate the process involved. hope this is helpful \$\endgroup\$ – Karthikeyan Oct 4 '13 at 7:10
7
\$\begingroup\$

To avoid the messy switch block, you should probably create an interface or abstract class called FileConversionHandler:

public abstract class FileConversionHandler {
    public abstract void process(File f);
}

Then, for each type of file, make a subclass that implements process() differently.

\$\endgroup\$
2
\$\begingroup\$

What is interesting about the java.io.File is that it is not final, which means that it can be subclassed to add behaviour. Object-Oriented Programming is all about behaviour, encapsulation, and information hiding. Often switch statements (or compounded if-else statements) can be replaced with a command pattern.

The reason it isn't immediately apparent how this can be accomplished is because the code, as written, violates information hiding. The violation occurs when clients to the File class need to determine the extension themselves and then use the extension to determine the file type. Consider the following:

public class ImageFile extends java.io.File {
  public boolean isImageType( ImageType imageType ) {
    return getImageType().equals( imageType );
  }

  public ImageType getImageType() {
    String extension = "OTHER";
    String fileName = getName();

    int i = fileName.lastIndexOf('.');

    // Probably an issue here, for the reader to fix. What happens
    // when there is only a dot, but no characters after the dot?
    if (i > 0) {
        extension = fileName.substring(i+1).trim().toUpperCase();
    }

    return ImageType.valueOf( extension );
  }
}

Note that getImageType() could use Apache Commons IO:

  public ImageType getImageType() {
    // Make sure getExtension never returns null!
    return ImageType.valueOf( FilenameUtils.getExtension( getName() ).toUpperCase() );
  }

How the extension is parsed becomes an abstraction that the calling code uses, rather than implements. This keeps behaviour with its data, a key concept of OOP. (As an aside, even Apache Commons gets this wrong, as evidenced by the FilenameUtils class, which isn't a true class, because it has no attributes.)

The answer from 200_success is almost correct. Ultimately, though, the FileConversionHandler would also need to know the file's extension, which unnecessarily imposes some constraints.

You can now create an ImageFileProcessor interface and implementations, which uses the command pattern and the factory pattern.

import java.util.HashMap;
import java.util.Map;

public ImageFileProcessorFactory {
  private static Map<ImageType, ImageFileProcessor> processors = new HashMap<>();

  static {
    // For processing PNG files...
    processors.put( ImageType.PNG, new PNGImageFileProcessor() );
  }

  public static ImageFileProcessor createProcessor( ImageFile imageFile ) {
    ImageFileProcessor ifp = processors.get( imageFile.getImageType() );
    ifp.setPrimaryFile( imageFile );
    return ifp;
  }
}

You could change the factory above to allow for processing a specific combination of files by using MapEntry and SimpleEntry instances. See this answer for details. Whichever implementation is required, you'd need to define:

public interface ImageFileProcessor {
  public void setPrimaryFile( ImageFile primaryFile );
  public void process( ImageFile other );
}

Then, the PNGImageFileProcessor becomes:

public class PNGImageFileProcessor extends AbstractImageFileProcessor {
  public void process( ImageFile other ) {
    // Process the PNG image.
  }
}

Along with an abstract implementation to avoid duplicate code:

public abstract class AbstractImageFileProcessor implements ImageFileProcessor {
  private ImageFile primary;

  public void setPrimaryFile( ImageFile primary ) { this.primary = primary; }
  protected ImageFile getPrimaryFile() { return this.primary; }
}

Then main reduces to:

  public static void main( String args... ) {
    ImageFileProcessor ifp = ImageFileProcessorFactory.createProcessor( args[1] );
    ifp.process( args[2] );
    System.out.println( "Done" );
  }

If you want to add more processors, it's trivial, and the main method never need change. Once the code is so crafted, it would be straightforward to dynamically add new image processors to the code base while the application is running.

Consider a text file such as:

JPG,JPGImageProcessor
JPEG,JPGImageProcessor
TIF,TIFFImageProcessor
TIFF,TIFFImageProcessor

Reloading this file would repopulate the processors command map:

    processors.put( ImageType.PNG, new PNGImageFileProcessor() );

All you would need to do is create a new XImageProcessor class (where X is the file type), ensure the class exists on the classpath, and then use reflection to dynamically instantiate the class.

The Factory would need a method to refresh itself, which could then be tied to a file monitor process that kicks off the refresh event automatically when the file has changed. So many possibilities.

At the risk of evoking ire, you could encapsulate the processor creation and execution mechanisms. In theory, you could make getImageType() a private method, then re-write main:

  public static void main( String args... ) {
    (new ImageFile( args[1] )).process( args[2] );
    System.out.println( "Done" );
  }

That is an exercise for the reader.

\$\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.