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