Properly designing an abstract class: How to retrieve data?

Question

I come from a self taught background and am trying to shore up my weaknesses in OOP, specifically C# and class design. I've been reading Code Complete 2 and it became apparent that I'm not following good class design principals. I currently have a ReportHandler class that generates a report based on a ReportType enum that is set when the handler is instantiated.

So now I'm refactoring the code, and am starting by considering the reports and how they differ:

They all share the following properties which will all be the same type:

• Report ID (Guid)
• Start Date (DateTime)
• End Date (DateTime)
• Total Results (int)

They also all have to retrieve the data from a database (currently the same database, could change in the future) and parse the data into an object, and be able to convert that object into a JSON. The data object and parsing will be different for each report.

So I'm wondering what the best design for this would be. If I created an abstract class I could do it like this:

public abstract class Report
{
    public abstract void GetData();
    public abstract void ParseData();
    public abstract string ToJson();

    public Guid ReportId { get; set; }
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }
    public int TotalResults { get; set; }

    //Not sure if this is correct, I want to make sure we can't instantiate a derived class without these values.
    protected Report(Guid reportId, DateTime startDate, DateTime endDate)
    {
        if (startDate > endDate)
            throw new ArgumentException("Start date cannot be after end date.");
        ReportId = reportId;
        StartDate = startDate;
        StartDate = endDate;
    }
}

I believe this would help keep the classes organized. I could create a something like this:

public class SpecificReport : Report
{
    //The raw data type can change between reports.
    private List<string> _rawData;
    public SpecificReportObject ParsedReport { get; private set; }

    public SpecificReport(Guid reportId, DateTime startDate, DateTime endDate)
        : base(reportId, startDate, endDate)
    {

    }

    public override void GetData()
    {
        var rawData = new List<string>();
        //Connect to a database and assign the results to rawData
        _rawData = rawData;
    }

    public override void ParseData()
    {
        var parsedReport = new SpecificReportObject();
        foreach (var result in _rawData)
        {
            //Parse RawData into a specific report object
        }
        ParsedReport = parsedReport;
    }

    public override string ToJson()
    {
        return JsonConvert.SerializeObject(ParsedReport);
    }

    public class SpecificReportObject
    {
        //Specific specific properties and logic
    }
}

Now that seems more organized to me, and logically seems to make more sense. You can instantiate a report, call GetData(), ParseData(), and finally ToJson() to get the report in a portable format.

However I am wondering if the derived reports should contain this functionality at all. I don't like that I have to remember to call GetData() and ParseData(), but putting those functions in the constructor seems like a bad idea. I wouldn't want to put something in a constructor that can fail, especially a potentially lengthy operation like querying a database.

Would it be better to separate GetData from the class? Then I could retrieve the data first and load it into the ParseData function as an argument

Answer 1

I'm going to make a different suggestion more to broaden your perspective. While I do recommend the concepts behind what I'll say generally, I would need more context to see if it is most appropriate for your situation (and likely it would suggest changes to other code).

You absolutely hit the nail on the head when you mention your concerns about having to remember to call methods. The situation is potentially worse. Even if I remember to call the methods, what happens if I call GetData again later? As your code is currently written, you end up with an object with inconsistent data. (As an aside, you should probably look at Dependency Injection [the concept formerly known as "parameterization"] to move details like where to fetch the data out of your classes. I won't focus on this below.)

One solution to this problem is to use the type system to help enforce your invariants. That's what it is there for. To make the example a little more impactful, I'm going to assume there is some pressing need to have explicit control over when a report is parsed. (In your actual code, it's not clear why you expose GetData at all. If you don't want it done in the constructor, then have it done on-demand by ParseData.) The key idea is:

Make a different type for each stage of processing.

Consider something like:

public interface IReportDataProvider {
    IUnparsedReport Fetch();
}

public interface IUnparsedReport {
    // ...
    IParsedReport Parse();
}

public interface IParsedReport {
    // ...
    string ToJsonString();
}

With this approach, you are now just unable to do things out of order, or forget to do a step, or produce inconsistent data by duplicating a step. Applying this technique doesn't actually require a type system. It's just data abstraction. One place where a type system can be valuable in this is one possible implementation of Parse in some contexts may be return this;. In this scenario, we are using the type system to enforce invariants because if you just ask for an IUnparsedReport, you don't know that the specific instance you get also implements IParsedReport (unless you do something grotty like a downcast or reflection). In fact, you can apply this approach to your code almost as-is. This can avoid needing wrappers or copying data around or duplicating code. I don't recommend doing this by default though as it implies mutation and can lead to hidden sharing that produces confusing results.

To cast this approach in OOD terms, you can view each stage as a Factory that produces the next stage. I used terminology similar to that in BobDalgleish's answer as the DataProvider discussed there can be viewed as a stage. (Though depending on how general it is, it may be better not to think of that way and just provide it as a parameter to a specific implementation of IUnparsedReport.) I don't agree with the description of the class though, as it just recapitulates the problem in the DataProvider. What if you forget to "connect"? You can use the same techniques in this answer to resolve the staging issues that the description of DataProvider brings up.

There's a broader mantra, of which this is an instance, popular in the typed functional programming community, though it's not at all limited to the FP or even the typed programming communities. Namely:

Make invalid states unrepresentable.

Answer 2

Would it be better to separate GetData from the class? Then I could retrieve the data first and load it into the ParseData function as an argument

This is the way to go. Provide a DataProvider class instance to the constructor for your Report sub-class. The DataProvider interface will offer methods for connecting/disconnecting and querying the data. As a bonus, this will allow you to build your test scaffold to test your reports with specific DataProvider instances that meet your testing needs.

Answer 3

They all share the following properties which will all be the same type

This should never be a criteria for defining an abstract class. Using this approach, you structural inheritance, which breaks encapsulation, is fragile and all in all procedural.

They also all have to retrieve the data from a database (currently the same database, could change in the future) and parse the data into an object, and be able to convert that object into a JSON.

That's way better! You identified your abstraction: what your reports should do.

interface Report
{
    public function initialize();

    public function parse();

    public function toJson();
}

However, as you mentioned, there is a temporal coupling in the way you use your report: I have to remember to call GetData() and ParseData(). I don't like it either. And I agree that constructors should be lightweight.

After identifying these problems I look closely at my abstraction. What is it all about? It's about getting and presenting data. That is a report's intrinsic behavior. So probably I could abstract away these operations. As you noted, there could be different data sources, and I suspect there are more than only json representation. So I have two separate hierarchies, representing data retrieval and data representation:

interface Report
{
    public function display();
}

interface DataStorage
{
    /**
     * @return array
     */
    public function get();
}

And the implementation could be like the following:

class ArrayedReport implements Report
{
    private $reportId;
    private $startDate;
    private $finishDate;
    private $storage;

    public function __construct(ReportId $reportId, DateTime $startDate, DateTime $finishDate, DataStorage $storage)
    {
        $this->reportId = $reportId;
        $this->startDate = $startDate;
        $this->finishDate = $finishDate;
        $this->storage = $storage;
    }

    public function display()
    {
        var_export(
            $this->storage->get(
                $this->reportId,
                $this->startDate,
                $this->reportId
            )
        );
    }
}

class JsonReport implements Report
{
    private $reportId;
    private $startDate;
    private $finishDate;
    private $storage;

    public function __construct(ReportId $reportId, DateTime $startDate, DateTime $finishDate, DataStorage $storage)
    {
        $this->reportId = $reportId;
        $this->startDate = $startDate;
        $this->finishDate = $finishDate;
        $this->storage = $storage;
    }

    public function display()
    {
        echo
            json_encode(
                $this->storage->get(
                    $this->reportId,
                    $this->startDate,
                    $this->reportId
                )
            );
    }
}

Now I notice that the criteria upon which data is retrieved could be abstracted away either. This can lead me to a third abstraction:

interface Criteria
{
    /**
     * @return array
     */
    public function value();
}

interface DataStorage
{
    /**
     * @param Criteria $criteria
     * @return array
     */
    public function get(Criteria $criteria);
}

interface ReportRepresentation
{
    public function display();
}

This can be implemented like the following:

class JsonReport implements ReportRepresentation
{
    private $criteria;
    private $storage;

    public function __construct(Criteria $criteria, DataStorage $storage)
    {
        $this->criteria = $criteria;
        $this->storage = $storage;
    }

    public function display()
    {
        echo json_encode($this->storage->get($this->criteria));
    }
}

Note, that I deliberately haven't introduce an abstraction called ReportData. It would be simply a data structure for holding a criteria and data storage, and definitely it would have a single implementation. Hence, it would be a useless abstraction.

And now, I have three abstractions with multiple implementations, that can be composed in a myriad ways for different purposes, including easy unit-testing. That is my criteria of good domain decomposition, the one that makes design truly solid.