Class design: file conversion logic and class design

Question

This is pretty basic, but sort of a generic issue so I want to hear what people's thoughts are. I have a situation where I need to take an existing MSI file and update it with a few standard modifications and spit out a new MSI file (duplication of old file with changes).

I started writing this with a few public methods and a basic input path for the original MSI. The thing is, for this to work properly, a strict path of calls has to be followed from the caller:

var custom = CustomPackage(sourcemsipath);
custom.Duplicate(targetmsipath);
custom.Upgrade();
custom.Save();
custom.WriteSmsXmlFile(targetxmlpath);

Would it be better to put all the conversion logic as part of the constructor instead of making them available as public methods? (in order to avoid having the caller have to know what the "proper order" is):

var custom = CustomPackage(sourcemsipath, targetmsipath); // saves converted msi
custom.WriteSmsXmlFile(targetxmlpath); // saves optional xml for sms

The constructor would then directly duplicate the MSI file, upgrade it and save it to the target location. The "WriteSmsXmlFile is still a public method since it is not always required.

Personally I don't like to have the constructor actually "do stuff" - I prefer to be able to call public methods, but it seems wrong to assume that the caller should know the proper order of calls?

An alternative would be to duplicate the file first, and then pass the duplicated file to the constructor - but it seems better to have the class do this on its own.

Maybe I got it all backwards and need two classes instead: SourcePackage, TargetPackage and pass the SourcePackage into the constructor of the TargetPackage?

Answer 1

I'd go with your first thought: put all of the conversion logic into one place. No reason to expose that sequence to users.

Incidentally, I agree with you about not putting actions into a constructor. I'd probably not do this in the constructor, and instead do it in a separate converter method, but that's personal taste.

Answer 2

It may be just me, but the thought of a constructor doing all these things makes me shiver. But why not provide a static method, which does all this:

public class CustomPackage 
{

    private CustomPackage(String sourcePath) 
    {
         ...
    }

    public static CustomPackage Create(String sourcePath, String targetPath) 
    {
         var custom = CustomPackage(sourcePath);
         custom.Duplicate(targetPath);
         custom.Upgrade();
         custom.Save();
         return custom;
    }
}

The actual advantage of this method is, that you won't have to give out an instance of CustomPackage unless the conversion process actually succeeded (safe of the optional parts).

Edit In C#, this factory method can even be used (by using delegates) as a "true" factory according to the Factory Pattern:

public interface ICustomizedPackage 
{
    ...
}

public class CustomPackage: ICustomizedPackage 
{
    ...
}

public class Consumer 
{
    public delegate ICustomizedPackage Factory(String,String);

    private Factory factory;

    public Consumer(Factory factory) 
    {
        this.factory = factory;
    }

    private ICustomizedPackage CreatePackage() 
    {
        return factory.Invoke(..., ...);
    }

    ...
}

and later:

new Consumer(CustomPackage.Create);

Answer 3

In such situations I typicaly use the following design:

var task = new Task(src, dst); // required params goes to constructor
task.Progress = ProgressHandler; // optional params setup
task.Run();

Answer 4

You're right to think that the constructor shouldn't do any more work than to simply initialize the object.

Sounds to me like what you need is a Convert(targetmsipath) function that wraps the calls to Duplicate, Upgrade and Save, thereby removing the need for the caller to know the correct order of operations, while at the same time keeping the logic out of the constructor.

You can also overload it to include a targetxmlpath parameter that, when supplied, also calls the WriteSmsXmlFile function. That way all the related operations are called from the same function on the caller's side and the order of operations is always correct.

Answer 5

I think there are service-oriented ways and object-oritented ways.

The service-oriented way would be to create series of filters that passes along an immutable data transfer object (entity).

var service1 = new Msi1Service();
var msi1 = service1.ReadFromFile(sourceMsiPath);
var service2 = new MsiCustomService();
var msi2 = service2.Convert(msi1);
service2.WriteToFile(msi2, targetMsiPath);
service2.WriteSmsXmlFile(msi2, targetXmlPath);

The object-oriented ways can use decorator pattern.

var decoratedMsi = new CustomMsiDecorator(new MsiFile(sourceMsiPath));
decoratedMsi.WriteToFile(targetMsiPath);
decoratedMsi.WriteSmsXmlFile(targetXmlPath);