c# design question

Question

I am just confused on the approach. Pls suggest me which is best. I will be creating multiple reports. SalesReport, ProfitReport etc.

Approach - 1:

class Report
{
  ReportType type;
}

Subclass ReportType as SalesType, ProfitType and assign it to report instances

SalesReport:

Report sales = new Report();
sales.type = new SalesType();

ProfitReport:

Report profit = new Report();
profit.type = new ProfitType();

Approach 2:

class Report
{
}

class SalesReport : Report
{
SalesType type;
}

class ProfitReport : Report
{
ProfitType type;
}

Which approach is best? and best design? Thanks a lot.

Every report will have different criterias, different output options like Email, print etc.

class Report
{
  ReportType type;
  Criteria criteria;
  Output output;
}

These classes are used like a Entity classes. For e.g from a browser/client we'll get the xml <Report><Type>Sales</Type><Criteria>...</Criteria></Report>. Based on the XML I need to form the Report classes and pass it for processing. Based on report type it will be executed.

Answer 1

You should create an Interface:

interface IReport {
    void RenderReport();
}

and then code like this:

class SalesReport : IReport {
    void RenderReport()
    {
         // Do something here
    }
}

class ProfitReport : IReport {
    void RenderReport()
    {
         // Do something here
    }
}

and then you can refer to either one, like so:

IReport report;
report = new ProfitReport();
report.RenderReport();
report = new SalesReport();
report.RenderReport();

Answer 2

What are the ReportType, SalesType, and ProfitType classes for? It's not good style to end class names with "Type", unless they identify the type of something else. For that case, an enum is almost always what you want:

enum ReportType{
    Sales,
    Profit,
    // ...
}

But, if you go with approach 2 (which is usually the best choice in this kind of situation), you really don't need a property to identify the report type, since you can use type identification for this purpose.

In other words, don't do this:

void DoSomethingWithReport(Report r){
    if(r == null)
        throw new ArgumentNullException();

    switch(r.Type){
    case ReportType.Sales:
        ((SalesReport)r).DoSomething();
        break;
    case ReportType.Profit:
        ((ProfitReport)r).DoSomethingElse();
        break;            
    }
}

Instead, do something like this:

void DoSomethingWithReport(Report r){
    if(r == null)
        throw new ArgumentNullException();

    SalesReport sr = r as SalesReport;
    if(sr != null){
        sr.DoSomething();
        return;
    }

    ProfitReport pr = r as ProfitReport;
    if(pr != null);
        pr.DoSomethingElse();
}

Even better, use polymorphism:

abstract class Report{
    public abstract void DoGeneralThing();
}

class SalesReport : Report{
    public override void DoGeneralThing(){
        /*Perform the old duties of SalesReport.DoSomething*/
    }
}

class ProfitReport : Report{
    public override void DoGeneralThing(){
        /*Perform the old duties of ProfitReport.DoSomethingElse*/
    }
}

void DoSomethingWithReport(Report r){
    r.DoSomething();
}

Answer 3

Generally, approach 2 is best. This allows polymorphic examination of the class type by the runtime when deciding what to do. Putting a discriminator in your class forces you to examine the exact type of report yourself, which has to be done at runtime. A method external to the Report hierarchy that takes a specific Report child will, for instance, be required to evaluate the discriminator, and will throw an exception at runtime if it was passed the wrong report type. With specific report children, the method can be defined as accepting only ProfitReports, and will either generate a compiler error if you attempt to give it a SalesReport, or with polymorphism you can specify overloads for SalesReport and ProfitReport and the compiler will use the correct method.

If the two reports are very similar, then most of the code should end up in the base class Report, with the derived types providing specific implementation details.

Answer 4

Are you going to be treating SalesReports and ProfitReports as Reports in any polymorphic ways?

If not, don't even have them inherit from a base class.

Is there much/any shared logic between a Sales report vs. a Profit report?

Inheritance for code reuse is an anti-pattern. If you have shared logic, consider composition.

Composition vs. Inheritance

Skip to the "When to use Inheritance and when to use Composition?" part first. It's a Java based article, but the concepts are pretty universal for static OO languages.

Answer 5

If say your Sales Report is going to have all of the same properties as Report then deriving them from a base class like that is the best way to go. However, if there are going to be extreme variations from one type of report to another then I would go the other way.

Essentially what I would see being in the report class is things like name and address. Then the actual statistics or data of the report would be stored in the SalesReport class. It all depends on what you need. What is important is to think about the hierarchy. What data is important to all reports? What data is specific to a certain type of report? Answer these questions and you will have your answer.

Answer 6

Approach 2 would be better since you can reuse code that's simmilar to all of the reports (print, reevaluate, etc...). And there's no need for the "SalesType" inside it since you're already subclassing...

This is also known as the "template pattern" in design patterns terminology.

Answer 7

As SalesType and ProfitType already derive from ReportType, I would say, approach #1.

Why? Approach #2 could lead to code duplication.