CODEWITHSUNDEEP
c#dictionaryenums
Coding Monkey
Coding Monkey

Reputation: 1000

Are Enum the right way to go here?

I'm not sure if I am abusing Enums here. Maybe this is not the best design approach.

I have a enum which declares the possible parameters to method which executes batch files.

public enum BatchFile
{
    batch1,
    batch2
}

I then have my method:

public void ExecuteBatch(BatchFile batchFile)
{
    string batchFileName;
    ...
    switch (batchFile)
        {
            case BatchFile.batch1:
                batchFileName = "Batch1.bat";
                break;
            case BatchFile.batch2:
                batchFileName = "Batch2.bat";
                break;
            default:
                break;
        }
    ...
    ExecuteBatchFile(batchFileName);
}

So I was wondering if this is sound design.

Another option I was thinking was creating a Dictionary<> in the constructor like this:

Dictionary<BatchFile, String> batchFileName = new Dictionary<BatchFile, string>();
batchFileName.Add(BatchFile.batch1, "batch1.bat");
batchFileName.Add(BatchFile.batch2, "batch2.bat");

Then instead of using a switch statement I would just go:

public void ExecuteBatch(BatchFile batchFile)
{
    ExecuteBatchFile(batchFileName[batchFile]);
}

I'm guessing the latter is the better approach.

Upvotes: 2

Views: 1008

Answers (10)

Mark
Mark

Reputation: 2432

If you want to use an enum then you may want to consider utilising attributes so you can store additional inforation (such as the file name) against the elements.

Here's some sample code to demonstrate how to declare the attributes:

using System;

public enum BatchFile
{
    [BatchFile("Batch1.bat")]
    batch1,
    [BatchFile("Batch2.bat")]
    batch2
}

public class BatchFileAttribute : Attribute
{
    public string FileName;
    public BatchFileAttribute(string fileName) { FileName = fileName; }
}

public class Test
{
    public static string GetFileName(Enum enumConstant)
    {
        if (enumConstant == null)
            return string.Empty;

        System.Reflection.FieldInfo fi = enumConstant.GetType().GetField(enumConstant.ToString());
        BatchFileAttribute[] aattr = ((BatchFileAttribute[])(fi.GetCustomAttributes(typeof(BatchFileAttribute), false)));
        if (aattr.Length > 0)
            return aattr[0].FileName;
        else
            return enumConstant.ToString();
    }
}

To get the file name simply call:

string fileName = Test.GetFileName(BatchFile.batch1);

Upvotes: 3

Guffa
Guffa

Reputation: 700322

The first solution (the switch) is simple and straight forward, and you really don't have to make it more complicated than that.

An alternative to using an enum could be to use properties that returns instances of a class with the relevant data set. This is quite expandable; if you later on need the Execute method to work differently for some batches, you can just let a property return a subclass with a different implementation and it's still called in the same way.

public class BatchFile {

   private string _fileName;

   private BatchFile(string fileName) {
      _fileName = fileName;
   }

   public BatchFile Batch1 { get { return new BatchFile("Batch1.bat"); } }
   public BatchFile Batch2 { get { return new BatchFile("Batch2.bat"); } }

   public virtual void Execute() {
      ExecuteBatchFile(_fileName);
   }

}

Usage:

BatchFile.Batch1.Execute();

Upvotes: 0

VVS
VVS

Reputation: 19604

What if you suddenly need a third batch file? You have to modify your code, recompile your library and everybody who uses it, has to do the same.

Whenever I find myself writing magic strings that might change, I consider putting them into an extra configuration file, keeping the data out of the code.

Upvotes: 4

Ken McCormack
Ken McCormack

Reputation:

Using enums is ok if you don't need to add new batch files without recompiling / redeploying your application... however I think most flexible approach is to define a list of key / filename pairs in your config.

To add a new batch file you just add it to the config file / restart / tell your user the key. You just need to handle unknown key / file not found exceptions.

Upvotes: 1

jrharshath
jrharshath

Reputation: 26583

The second approach is better, because it links the batch file objects (enums) with the strings..

But talking about design, it would not be very good to keep the enum and the dictionary separate; you could consider this as an alternative:

public class BatchFile {
    private batchFileName;

    private BatchFile(String filename) {
        this.batchFileName = filename;
    }
    public const static BatchFile batch1 = new BatchFile("file1");
    public const static BatchFile batch2 = new BatchFile("file2");

    public String getFileName() { return batchFileName; }
}

You can choose to keep the constructor private, or make it public.

Cheers,

jrh.

Upvotes: 0

Mitch Wheat
Mitch Wheat

Reputation: 300549

I'd probably go for a design along these lines:

public interface IBatchFile
{
    void Execute();
}

public class BatchFileType1 : IBatchFile
{
    private string _filename;

    public BatchFileType1(string filename)
    {
        _filename = filename;
    }

    ...

    public void Execute()
    {
        ...
    }
}

public class BatchFileType2 : IBatchFile
{
    private string _filename;

    public BatchFileType2(string filename)
    {
        _filename = filename;
    }

    ...

    public void Execute()
    {
        ...
    }
}

In fact, I'd extract any common functionality into a BatchFile base class

Upvotes: 8

Mark Cidade
Mark Cidade

Reputation: 99957

I would personally use a static class of constants in this case:

public static class BatchFiles
 { 
   public const string batch1 = "batch1.bat";
   public const string batch2 = "batch2.bat"; 
 }

Upvotes: 3

Jay Atkinson
Jay Atkinson

Reputation: 3287

The problem with the latter case is if something passed an invalid value that is not inside the dictionary. The default inside the switch statement provides an easy way out.

But...if you're enum is going to have a lot of entries. Dictionary might be a better way to go.

Either way, I'd recommend some way to provide protection of the input value from causing a bad error even in ammoQ's answer.

Upvotes: 0

JaredPar
JaredPar

Reputation: 754715

I think the latter approach is better because it separates out concerns. You have a method which is dedicated to associating the enum values with a physical path and a separate method for actually executing the result. The first attempt mixed these two approaches slightly.

However I think that using a switch statement to get the path is also a valid approach. Enums are in many ways meant to be switched upon.

Upvotes: 2

Erich Kitzmueller
Erich Kitzmueller

Reputation: 36977

Is it really necessary that ExecuteBatch works on limited number of possible file names only? Why don't you just make it

public void ExecuteBatch(string batchFile)
{
    ExecuteBatchFile(batchFile);
}

Upvotes: 0

Related Questions