Writing a method based on the value of an enumeration without falling into a code smell

Question

Imagine I have a document (word document).

I have an enumeration which will indicate how to extract data from the document. So if I want just text, the images, or both (3 members of the enumeration).

I have a case statement based on this enumeration, but without falling into a code smell, how can I write code which isn't too repetitive? For every condition in the switch, should I have a seperate method (the easiest way), or a method accepting a paremeter (like the value of the enumeration), and then use if statements to say if(xyz) do abc, and so on.

Or is there a quicker, more efficient way?

Thanks

Answer 1

I would use a Strategy pattern coupled with a factory to create the appropriate strategy based on the value of the enumeration. EDIT As others have pointed out you could also determine the correct strategy via a Map as well. Factory is my choice because it only encapsulates the logic and doesn't require any data storage.

public interface IExtractionStrategy
{
    object Extract( Document doc );  // or what ever result is best
}

public class TextExtractionStrategy : IExtractionStrategy
{
    public object Extract( Document doc )
    {
     .... algorithm for extracting text...
    }
}

public class ImageExtractionStrategy : IExtractionStrategy
{
    public object Extract( Document doc )
    {
     .... algorithm for extracting images...
    }
}


public static class StrategyFactory
{
     IExtractionStrategy GetStrategy( ExtractionEnum strategyType )
     {
         switch (strategyType)
         {
             case ExtractionEnum.Text:
                 return new TextExtractionStrategy();
                 break;
             case ExtractionEnum.Image:
                 return new ImageExtractionStrategy();
                 break;

             ...
         }
     }
}

Answer 2

A solution could be having one class for every way you want to extract the data. Each class would do all the extraction work and they could have a common IExtract interface. You would only have to return the interface based on the enumeration and then just call the methods on the interface.

Answer 3

Use a strategy pattern to decouple the method of data processing from its selection.

You could then use a straight Map<Enum, Strategy> to hold the relationship from enumeration to extraction strategy, or even have each instance of the enumeration hold a reference to its own strategy.

Answer 4

Hi,

Since it is hard to give a general statement without knowing your exact code I can only recommend you to read this article:

Back to Basics - Life After If, For and Switch - Like, a Data Structures Reminder by Scott Hanselman.

Since you only have 3 values in your enum any pattern like the strategy pattern or a map might be an architectural overkill (don't get me wrong, these patterns can be very useful on larger enums). It's all about choosing the right solution for a specific problem.

Regards, divo

Answer 5

How many classes does it take to change a lightbulb?

Perhaps it's just a terminology difference, but this seems to me a simple case of needing a dispatch table:

use constant EXTRACT_TEXT => 1, EXTRACT_IMAGES => 2, EXTRACT_BOTH => 3;
my %extractor = (
    (EXTRACT_TEXT) => \&extract_text,
    (EXTRACT_IMAGES) => \&extract_images,
    (EXTRACT_BOTH) => \&extract_both,
);
...
die "no extractor found for $enum_value" if ! $extractor{ $enum_value };
$extractor{ $enum_value }->( $document_info );

Answer 6

I would do (pseudocode):

switch(enum)
   case images:
       extractImages();
       break;
   case text:
       extractText();
       break;
   case both:
        extractImages();
        extractText();
        break;

Answer 7

Thanks for the advice guys, that definately answers my question and helps my design! :)

Answer 8

If you're using Java enumerations, then you can make use of the fact that they are actually objects:

enum ExtractionEnum {
    IMAGE {
        public byte[] extract(InputStream is) { ... }
    },

    TEXT {
        public byte[] extract(InputStream is) { ... }
    };

    public abstract byte[] extract(InputStream is);
}

// ...
public void doSomething(ExtractionEnum type) {
    byte[] data = type.extract(getInputStream());
    ...
}

I'm not sure if I like this better than the strategy pattern or good-ol' switch/case, but it works.