Need advice for refactoring a large class
Our C# application produces reports of various types. Each type of report has an xml schema defining the options and features that are available, so a user can create an xml document describing the report they want. There are elements that are common to multiple report types, and there are interdependencies among some elements.
We currently have a single class full of static methods to handle the parsing of the xml. It will take a schema-validated document, and return an object representing the type of report and its configured options. The class looks something like this:
public class ReportFactory
{
//let's say 4 types of reports
public static ReportType1 CreateReportType1(XDocument document)
{
//logic to create this type of report, calling private methods in this class
}
....
public static ReportTypeN CreateReportTypeN(XDocument document)
{
//logic to create this type of report
}
//several dozen private methods, which get called from the public methods or from each other
private static Feature1 CreateFeature1(XElement element)
{
//create some feature of a report
}
private static FeatureN CreateFeatureN(XElement element, FeatureM m)
{
//create some feature which relies on another previously built feature
}
private static FeatureX CreateFeatureX(ReportType2 report, XElement elementForX, XElement elementRelatedToX)
{
//create some feature, which relies on
//more than one elem开发者_StackOverflowent and/or a partially built report of a given type
}
private static void UpdateFeatureNWithStuffFromFeatureM(FeatureN n, FeatureM m)
{
//modify parts of the built report, based on some other parts of the report
}
...
}
I believe the intention was to encapsulate the details of the xml structure, which I suppose it does. It also doesn't suffer much from duplicated code. But it is very large and hard to read, and it gets worse as we add more features. It's also difficult to test, since it relies heavily on things being done in the correct order.
I'd like to refactor it, but so far the only thing I can think of is to just split it up into multiple classes; say, a class for each report type and an additional helper class for common stuff. But it will still be messy and possibly even harder to read. Is there a good way to organize something like this? Any patterns that might help? I've looked at a bunch of creation patterns and haven't really found anything that seems to fit.
UPDATE: Sorry I have not had time or budget to actually work on this piece of refactoring, but thanks for the suggestions. The more I think about it, the more I like (something like) the Chain of Responsibility idea. The starting point (public function) will create the return object and fill in some basic stuff, then hand off the object and the xml to the next piece. Each piece will know which parts of the object and the xml it needs, and each piece can be tested independently by looking at the changes to the object.
I would guess that you're going to need several patterns and principles to enhance your class(es). You are already using the factory pattern. The problem you seem to be describing though is handling the specific order of activities and breaking your classes down into smaller chunks.
In order to handle the ordering of activities, it looks like you could use a chain of responsibility pattern..
On the note of testability of "portions" of the solution because of dependencies, part of that is test setup (you need better mocks or stubs to support the activities of a method) and perhaps better design that would hopefully come out of your efforts to test the application.
I would hope that there is some way to take advantage of inheritance and use proper constructors instead of a factory (I don't see the need for a factory). Also, it would be nice to put report-specific features only in the reports to which they apply, and feature-specific functions only in the features to which they apply. Something like this:
public class Report
{
protected Report(XDocument document)
{
// Any code common to creating the majority of reports
}
protected class Feature
{
private XElement element;
public Feature(XElement element)
{
// Any code common to creating the majority of features
}
public Feature(Feature feature)
{
element = feature.element;
// Any code common to creating features from other features
}
}
public class Feature1 : Feature
{
public Feature1(XElement element)
: base(element)
{
// Any code specific to creating Feature1
}
}
public class FeatureN : Feature
{
public FeatureN(FeatureM feature) : base(feature)
{
// Any code specific to creating a featureN from a featureM
}
public void UpdateFromFeatureM(FeatureM feature)
{
//modify parts of the built report, based on some other parts of the report
}
}
public class FeatureM : Feature
{
}
}
public class ReportType1 : Report
{
public ReportType1(XDocument document)
: base(document)
{
// Code specific to creating ReportType1
}
}
public class ReportType2 : Report
{
public ReportType2(XDocument document)
: base(document)
{
// Code specific to creating ReportType2
}
public class FeatureX
{
public FeatureX(ReportType2 report, XElement element, XElement relatedElement)
{
//create some feature, which relies on
//more than one element and/or a partially built report of a given type
}
}
}
A fairly common approach would be to use the Strategy pattern. You could have a IReportBuilder Interface which returned an IReport. Each concrete report builder would then encapsulate the specifics of building a report (maybe with a base class for common code). Similarly, the 'Features' could follow this pattern, with the appropriate features concrete class injected into a concrete Report.
I'm not sure how the client selects the reports; but a factory could be used to instantiate the correct concrete report builder, based off a key (this might be better with an example).
I would try the following
- Create
ReportType
abstraction and respective concrete classes. The return type ofCreateReportTypeX()
will be the abstract type. Report enumeration could help unifyingCreateReportX()
into a singleCreateReport()
- Similarly
Feature
abstraction and concrete Feature classes.A parameter object and Feature enumeration for forCreateFeatureX()
. ReportFactory
should deal withCreateReport()
only. We probably needFeatureFactory
forCreateFeature()
.
精彩评论