- He implemented 4 different ways of connecting to the DB instead of one standardized method for all reports [no, there was no good reason for doing what he did, one standardized method would work for all in our situation].
- He implemented 3 different ways of retrieving data, using 3 different types of objects instead of standardizing. One method used a standard .Net data object that would attempt to lock all the records in the DB while report was loading, which created major headaches for reports that took a while to generate.
- He used in-line SQL in the DLL - yes, I know, there are different schools of thought on this subject, but my team has a specific policy that dictates that developer's applications only use stored procedures and this is considered good practice in many circles.
- He created a query for each field in the report header that had to retrieve common header data.
- In every report, he had code for retrieving report footer data for stuffing into a text box in the footer of the report.
- He had no standard convention for naming parameters which were used in most reports. For example, most reports would require a "customerId" parameter, yet the variable would be coded as any one of the following (as an example): cust / customerID / custId / cusomterAccountKey / accountKey / customerAccountNumber -- I guess some people like variety.
To mitigate some of these issues I simply created a class named "StandardFormatReport" which inherits from Telerik.Reporting.Report:
public class StandardFormatReport : Telerik.Reporting.Report
To mitigate the first issue, I added a property to the StandardFormatReport which would return the one and only connection string the app is supposed to use according to the one and only approved method for retrieving connection strings:
protected string SqlConnectionString
{
get { return Conf.GetDBConnectionString(); }
}
In order to make sure that I can more easily know where to look to audit how the report is setting the data source, I added the following virtual function, which the developer would have to override in his report:
protected virtual void SetDataSource()
{
throw new System.Exception("The report class which inherits from RRFS.Reports.StandardFormatReport must override SetDataSource()");
}
All I would need to do in a code review process is look for where the developer would override the function to set the datasource, and make sure that he was connecting to the database correctly:
protected override void SetDataSource()
{
this.sqlDataSource1.ConnectionString = this.SqlConnectionString;
....
}
Lastly, the SetDataSource function is the logical place to set the reports parameters, which the developer named every different way under the sun with a string. This drives me nuts, not just because of the lack of consistency, but also because the developer could easily typo the param name and the report would not work. I deplore the using strings where constants should be used instead. To remedy this, I added the necessary constants which would define the standardized way to name params, in the StandardFormatReport class:
protected static class StandardParams
{
public const string CollectionAccountKey = "@CustomerAccountId";
public const string AssociationKey = "@CompanyId";
}
Thus instead of the developer using the following code:
protected override void SetDataSource()
{
this.sqlDataSource1.ConnectionString = this.SqlConnectionString;
this.sqlDataSource1.Parameters.AddRange(new Telerik.Reporting.SqlDataSourceParameter[] {
new Telerik.Reporting.SqlDataSourceParameter("@CollectionAcountKey", System.Data.DbType.Int32, _collectionKey)});
this.DataSource = sqlDataSource1;
}
[notice, I deliberately spelled the parameter incorrectly in this case, because this is highlights the typo issue]
... he would have to use the following code:
protected override void SetDataSource()
{
this.sqlDataSource1.ConnectionString = this.SqlConnectionString;
this.sqlDataSource1.Parameters.AddRange(new Telerik.Reporting.SqlDataSourceParameter[] {
new Telerik.Reporting.SqlDataSourceParameter(StandardParams.CollectionAccountKey, System.Data.DbType.Int32, _collectionKey)});
this.DataSource = sqlDataSource1;
}
You cant typo that and get your app to compile.
Finally, for all the instances of reports that had certain repeated fields that the developer had coded into each report, I added the following function to the StandardFormatReport class:
public void SetStandardFields()
{
Telerik.Reporting.TextBox tb = null;
tb = this.Items.Find("txtFooterDate", true)[0] as Telerik.Reporting.TextBox;
if (tb != null) tb.Value = "Information as of " + DateTime.Now.ToString("mm/dd/yy");
tb = this.Items.Find("txtFooterNote", true)[0] as Telerik.Reporting.TextBox;
if (tb != null) tb.Value = GlobalFunctions.GetFooterNote();
tb = this.Items.Find("txtFooterAddress", true)[0] as Telerik.Reporting.TextBox;
if (tb != null) tb.Value = GlobalFunctions.GetFooterAddress();
}
Then in the ctor for the StandardFormatReport, I simply add the call to SetStandardFields, and if they exists with the correct names, they will be set, this eliminating the need to have this coded in every single report -- standard DRY compliance issues.
There is more that one could do to put standard functionality into a class that derives from Telerik.Reporting.Report and enforce standardization through everyday OO programming, but I wont go into detail. The point is this: just because the Report Wizard will create a Telerik.Reporting.Report class for you in Visual Studio, it does not mean that you have to use it as is. It's still basically a C# object, subject to all the benefits that come with the OOP feature C# offers. Since most reports for business applications usually need common functionality, it only makes sense to put all that functionality into one custom-class that derives from Telerik.Reporting.Report, then have your developers create their reports by deriving from your custom-class.
I would argue that if you have a large reporting project where there are many typical elements (a common logo, same size, position, etc) required on just about every report, and you're not doing it this way, but repeating your code per report instead of inheriting from a class that already has the correct code, then you're doing it wrong.
No comments:
Post a Comment