c# programming code optimization

Question

I have about 20 classes which are derived from ConvertApi classes. Every class share Convert method from parent class and has unique property SupportedFiles. I use these classes for file manipulation and my code looks like

if (fileEx=="txt")
   new Text2Pdf().Convert(file)
else
  if (fileEx=="doc")
    new Word2Pdf().Convert(file)
     else
    //and so on..

I know that these code can be optimized because 20 times if operator is repeated and that looks bad, but can't find a way to do that. Could anyone help me?

class Text2Pdf : ConvertApi
{
  enum SupportedFiles { txt, log }; 
}

class Word2Pdf : ConvertApi
{
  enum SupportedFiles { doc, docx }; 
}

class Excel2Pdf : ConvertApi
{
  enum SupportedFiles { xls, xlsx }; 
}

class ConvertApi
{
 public void Convert(....);
}

First of all, change all sorts of `if (fileEx="txt")` to `if (fileEx=="txt")`. EDIT: You've edited your question. That's fine now. — Ofer Zelig, Mar 16 '12 at 11:23
One of the cases where the lack of virtual class methods in C# is annoying. — CodesInChaos, Mar 16 '12 at 11:25
Are those enum really Properties? Aren´t those nested enum types of each class? — Glenn, Mar 16 '12 at 11:26
I added enum as example, no matter that is used in my case. I just want to show the problem. — Tomas, Mar 16 '12 at 11:27
One possibility is to have a factory helper class to return the appropriate ConvertApi instance where either you do a huge switch based on the extension or alternatively create a dictionary mapping each extension to a ConvertApi instance in the factory and then have a method to return the appropriate instance based on the current extension. Depends how many times you need to use the factory as to which is a better approach — kaj, Mar 16 '12 at 11:34

Moo-Juice · Accepted Answer · 2012-03-16T11:38:47.567

In your base class, have something like this:

public abstract class ConvertApi
{
    protected abstract string[] SupportedFilesImpl();

    public bool Supports(string ext)
    {
        return SupportedFilesImpl.Contains(ext);
    }
}

Now, your derived classes can implement this method:

public class Text2PDF : ConvertApi
{
    protected override string[] SupportedFilesImpl { return new string[] { "txt", "log"}; }
}

public class Doc2PDF : ConvertApi
{
    protected override string[] SupportedFilesImpl { return new string[] { "doc", "docx"}; }
}

... and so on for the rest of the converters. Then put these in a list...

List<ConvertApi> converters = new List<ConvertApi>();
converters.Add(new Text2PDF());
converters.Add(new Doc2PDF());

(Note, I'd probably have a class containing these rather than just a list, but anyway). Now, to find a converter:

foreach(ConvertApi api in converters)
{
    if(api.Supports(fileExt))
    {
        // woo!
        break;
    }
}

This solution do not work in my case because I am loosing possibility to access Child Class properties. — Tomas, Mar 16 '12 at 12:17
@Tomas, if you need to access child properties at this point, then your design is screwed. :) — Moo-Juice, Mar 16 '12 at 12:46
I just solved the problem using dynamic keyword and your provided code. — Tomas, Mar 16 '12 at 13:13

score 2 · Answer 2 · answered Mar 16 '12 at 11:28

2

Assuming each converter is stateless, it sounds like you just want a Dictionary<string, ConvertApi>:

private static readonly Dictionary<string, ConvertApi> ConverterByType =
    new Dictionary<string, ConvertApi>
{
    { "txt", new Text2PdfConverter() },
    { "doc", new Word2PdfConverter() },
    ...
};

...

ConvertApi converter;
if (!ConverterByType.TryGetValue(fileEx, out converter))
{
    // No converter available... do whatever
}
else
{
    converter.Convert(file);
}

(The dictionary initialization will end up creating more converters than you really need for any converter that supports multiple extensions, but that's a different matter.)

If you need a new converter each time, make it a Dictionary<string, Func<ConvertApi>> and populate it as:

{ "txt", () => new Text2PdfConverter() },
{ "doc", () => new Word2PdfConverter() },

... then call the delegate to get the converter when you need it.

Of course, this puts all the initialization in one place - you may want a way of getting the converters to register the extensions they understand with a "converter provider" of some type.

answered Mar 16 '12 at 11:28

Jon Skeet

1,421,763
867
9,128
9,194

2

What happens if a converter supports multiple file extensions? :) – Moo-Juice Mar 16 '12 at 11:29
@Moo-Juice: Was editing for that - you end up with multiple instances, unless you take action to avoid it. It's definitely avoidable, but I'm going for the simplest options first. – Jon Skeet Mar 16 '12 at 11:31
This solution do not work in my case because I am loosing possibility to access Child Class properties. Your code return ConvertApi Parent class but I need to bet Child class because child class will contain additional properties and methods. – Tomas Mar 16 '12 at 12:20
@JonSkeet But then he is still populating a collection with every type. Whenever a new ConvertApi is made or one of them supports a new extension, the code must be updated. I tried to adress this issue in [my attempt](http://stackoverflow.com/a/9736522/677014) – Louis Kottmann Mar 16 '12 at 12:42
@Baboon: Yes, that's what the final paragraph is about. – Jon Skeet Mar 16 '12 at 14:06

David Brabant · Answer 3 · 2012-03-16T12:49:42.527

You need to use an abstract factory pattern, here. All your text converters should derive from a common interface ITextConverter implementing your Convert method. The file extension would be a parameter of your factory. Here is a sample below (typed "on the fly", sometimes copy-pasting code from my sources, so there might be typos. The goal here is to give you a general idea for a flexible implementation).

public interface IFileConverter
{
    bool Convert(string filePath);
}


public static class FileConverterFactory
{
    public static IFileConverter Create(string extension)
    {
        extension = type.ToUpper();

        Dictionary<string, ConverterConfig> availableConverters = GetConvertersConfig();

        if (!availableConverters.ContainsKey(extension))
            throw new ArgumentException(string.Format("Unknown extenstion type '{0}'. Check application configuration file.", extension));

        ConverterConfig cc = availableConverters[extension];
        Assembly runnerAssembly = Assembly.LoadFrom(cc.Assembly);
        Type converterType = runnerAssembly.GetType(cc.Class);

        IFileConverter converter = (IFileConverter) Activator.CreateInstance(converterType);

        return converter;
    }


    private static Dictionary<string, ConverterConfig> GetConvertersConfig()
    {
        var configs = (Dictionary<string, ConverterConfig>) ConfigurationManager.GetSection("ConvertersConfig");

        return configs;
    }
}


public class ConvertersConfigHandler : IConfigurationSectionHandler
{
    public object Create(object parent, object configContext, XmlNode section)
    {
        Dictionary<string, ConverterConfig> converters = new KeyedList<string, ConverterConfig>();
        XmlNodeList converterList = section.SelectNodes("Converter");

        foreach (XmlNode converterNode in converterList)
        {
            XmlNode currentConverterNode = converterNode;

            ConverterConfig cc = new ConverterConfig();
            cc.Type = XML.GetAttribute(ref currentConverterNode, "Type").ToUpper();
            cc.Assembly = XML.GetAttribute(ref currentConverterNode, "Assembly");
            cc.Class = XML.GetAttribute(ref currentConverterNode, "Class");

            converters[cc.Type] = cc;
        }

        return converters;
    }
}


public class ConverterConfig
{
    public string Type = "";
    public string Assembly = "";
    public string Class = "";
}


public class TextConverter : IFileConverter
{
   bool Convert(string filePath) { ... }
}


public class PdfConverter : IFileConverter
{
   bool Convert(string filePath) { ... }
}

In your app.config file, you add this to the configSections:

<section name = "ConvertersConfig" type = "ConvertersConfigConfigHandler, MyAssembly" />

and this below your configSections:

<ConvertersConfig>
    <Converter Type="txt" Assembly="MyAssembly" Class="MyAssembly.TextConverter" />
    <Converter Type="pdf" Assembly="MyAssembly" Class="MyAssembly.PdfConverter" />
</ConvertersConfig>

The call would then be like this:

IFileConverter converter = FileConverterFactory.Create("txt");
converter.Convert("c:\temp\myfile");

EDITED the code for giving a solution that is more "generic".

It would be best, imho, if the `ITextConverter` here defined a property to obtain which files it is able to convert, otherwise the "available" conversions are tied intrinsically to the factory, when it is the converters themselves that know this information. — Moo-Juice, Mar 16 '12 at 11:41
@Moo-Juice: you are right, of course. Just typed the code on the fly to show a simple example. In fact, if I had to implement the thing myself, I would use dependency injection + abstract factory. — David Brabant, Mar 16 '12 at 11:43

Sergey Kudriavtsev · Answer 4 · 2012-03-16T11:40:25.940

C# supports switch-case operator for strings, i.e. your code coud be rewritten as

switch (fileEx)
{
    case "txt" : new Text2Pdf().Convert(file); break;
    case "doc": new Word2Pdf().Convert(file); break;
    ...
}

If you change your classes' names to correspond to supported extensions then you will be able to construct them using reflection, like this (error checking omitted for brevity):

    var t = Type.GetType(fileEx + "2Pdf");
    var tConstructor = t.GetConstructor(Type.EmptyTypes);
    var tInstance = tConstructor.Invoke(new object[0]);
    ((ConvertApi)tInstance).Convert(...);

This might require some additional work (i.e. create a separate class for each extension, deriving them from some base class - for example Doc2Pdf and Docx2Pdf both deriving from Word2Pdf).

The advantage is that you will not have to touch this part of code anymore. If you're planning to write some interface for plugins then it may come in handy.

The code above also has an assumption that your ConvertApi classes all have default parameterless constuctor.

Louis Kottmann · Answer 5 · 2012-03-16T13:07:09.643

1

You can use some reflection:

ConvertApi FindMatchingConverter(string _FileExt)
        {
            //Get all the converters available.
            List<ConvertApi> converters = this.GetType()
                                           .Assembly
                                           .GetExportedTypes()
                                           .Where(type => type.IsAssignableFrom(typeof(ConvertApi)))
                                           .ToList();

            //Loop and find which converter to use
            foreach (var converter in converters)
            {
                if (converter.SupportedFiles.Contains(_FileExt))
                    return Activator.CreateInstance(converter);
            }
            throw new Exception("No converter found");
        }

Then you just have to call Convert() on the ConvertApi returned.
Of course, this requires you to add a virtual List<String> named SupportedFiles in your base class.

This makes it look like

public abstract class ConvertApi
{
    public abstract void Convert();

    public virtual List<String> SupportedFiles {get;set;}
}

edited Mar 16 '12 at 13:07

answered Mar 16 '12 at 11:36

Louis Kottmann

16,268
4
64
88

`Assembly.GetExportedTypes()` returns an array of `Type` and `Type` is NOT a `ConvertApi`. The idea is good but correct implementation will be a bit more complex. – Sergey Kudriavtsev Mar 16 '12 at 11:50
Still, you're assigning a `List` to `List converters`. This will throw compiler error "Can not assign... incompatible types". To do this correctly you can either return a `Type` or first instantiate return object similarly to my answer's code. – Sergey Kudriavtsev Mar 16 '12 at 12:43
@SergeyKudriavtsev right again, I updated the answer to use `Activator.CreateInstance()` and use a `virtual List` instead of an enum, since enums cannot be made virtual. – Louis Kottmann Mar 16 '12 at 12:49

score 0 · Answer 6 · answered Mar 16 '12 at 11:26

0

What about using a switch?

switch(fileEx){
    case "txt": new Text2Pdf().Convert(file); break;
    case "doc": new Word2Pdf().Convert(file); break;
}

answered Mar 16 '12 at 11:26

JP Hellemons

5,977
11
63
128

3

Not recommended - it's an [anti-pattern](http://stackoverflow.com/questions/505454/large-switch-statements-bad-oop/505555#505555). – Ofer Zelig Mar 16 '12 at 11:30
You should leverage OO polymorphysm. i.e. `ConvertAPI x = new Text2Pdf(); x.Convert(file);` but that's a short answer and the longer answer is by using a factory or Dependency Injection, but I can't elaborate on that here because I'm on my mobile phone. – Ofer Zelig Mar 16 '12 at 11:34

score 0 · Answer 7 · answered Mar 16 '12 at 11:26

0

Use dependency injection where you pass in the supported files in the constructor of the class.

answered Mar 16 '12 at 11:26

Andy Stannard

1,673
2
18
32

Chris Gessler · Answer 8 · 2012-03-16T11:57:22.553

Slower, but more dynamic... use an Object Factory. Here's a good article that seems to fit your needs.

http://www.codeproject.com/Articles/12986/Generic-Object-Factory

The important stuff from the article:

using System.Collections.Generic;

public struct Factory < KeyType, GeneralProduct > 
{
    //Register a object with the factory
    public void> Register< SpecificProduct >(KeyType key)
        where SpecificProduct : GeneralProduct, new()
    {
        if( m_mapProducts == null )
                {
            m_mapProducts = new SortedList< KeyType, CreateFunctor >(); 
        }
        CreateFunctor createFunctor = Creator<SpecificProduct>; 
        m_mapProducts.Add(key, createFunctor);
    }

    //Create a registered object 
    public GeneralProduct Create( KeyType key )
    {
        CreateFunctor createFunctor = m_mapProducts[ key ];
        return createFunctor(); 
    }

    private GeneralProduct Creator < SpecificProduct >() 
        where SpecificProduct : GeneralProduct, new()
    {
        return new SpecificProduct();
    }

    private delegate GeneralProduct CreateFunctor(); 

    private SortedList<KeyType, CreateFunctor>  m_mapProducts;
}

Usage:

class Fruit
{ 
} 

class Apple : Fruit 
{ 
} 

class Orange : Fruit 
{ 
} 

class TestClass
{ 
    static void Main(string[] args) 
    { 
        General.Factory< string, Fruit > factory; 

        //Register
        factory.Register< Apple >( "Apple" ); 
        factory.Register< Orange>( "Orange" ); 

        //Create 
        Fruit fruit1 = factory.Create("Apple"); 
        Fruit fruit2 = factory.Create("Orange"); 
    } 
}

Can you please explain the contents of the link, so in 3 years when the link is dead, somebody can get some assistance out of your post? — Security Hound, Mar 16 '12 at 11:52
You are SOOO right... I hate it when I click on dead links, and here I was about to create one. LOL — Chris Gessler, Mar 16 '12 at 11:54

c# programming code optimization

8 Answers8