Class Design: File Conversion Logic and Class Design
This is a fairly simple but common problem, so I want to hear what people think. I have a situation where I need to take an existing MSI file and update it with a few standard modifications and spit out the new MSI file (duplicate the old file with changes).
I started writing this with a few public methods and a basic input path for the original MSI. The point is that a strict call path must be followed in order to work properly with the caller:
var custom = CustomPackage(sourcemsipath);
custom.Duplicate(targetmsipath);
custom.Upgrade();
custom.Save();
custom.WriteSmsXmlFile(targetxmlpath);
Wouldn't it be better to put all the conversion logic as part of the constructor instead of making them available as public methods? (to avoid the caller knowing what the "correct order" is):
var custom = CustomPackage(sourcemsipath, targetmsipath); // saves converted msi
custom.WriteSmsXmlFile(targetxmlpath); // saves optional xml for sms
The designer will then directly duplicate the MSI file, update it, and save it to the target location. "WriteSmsXmlFile is still a public method because it is not always required.
Personally, I don't like it when the constructor actually "did things" - I prefer to be able to call public methods, but it seems wrong to assume that the caller needs to know the correct order of calls?
An alternative would be to duplicate the file first and then pass the duplicated file to the constructor - but it looks like the class should do this on its own.
Maybe I got it all back and you want two classes: SourcePackage , TargetPackage and pass SourcePackage to the TargetPackage constructor?
source to share
It might just be me, but the thought of a constructor doing all of this makes me shiver. But why not provide a static method that does all of 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 issue an instance CustomPackage
if the conversion process really failed (safe for extra parts).
Edit . In C #, this factory method can be used (using delegates) as a "true" factory according to Factory Sample :
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);
source to share
I would think: put all the conversion logic in one place. There is no reason to expose this sequence to users.
By the way, I agree with you that you are not inserting actions into the constructor. I would probably not do this in the constructor, but instead do it in a separate converter method, but that's personal taste.
source to share
You are correct in thinking that a constructor shouldn't do more work than just initialize an object.
It seems to me that you want a function Convert(targetmsipath)
that wraps calls to Duplicate
, Upgrade
and Save
thus eliminates the need for the caller to know the correct order of operations, while at the same time keeping the logic outside the constructor.
You can also overload it by including a parameter targetxmlpath
that, when enabled, also calls the function WriteSmsXmlFile
. This way, all related operations are called from the same function on the caller's side, and the order of the operations is always correct.
source to share
I think there are service oriented ways and object oriented ways.
A service oriented way would be to create a series of filters that traverse 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);
Object oriented ways can use the decorator pattern .
var decoratedMsi = new CustomMsiDecorator(new MsiFile(sourceMsiPath));
decoratedMsi.WriteToFile(targetMsiPath);
decoratedMsi.WriteSmsXmlFile(targetXmlPath);
source to share