2
\$\begingroup\$

After reading that Data Classes can be considered a code smell, I am shifting from a pattern with many data classes and a single manager class that handles all the instantiation of the classes, to a pattern where each data class becomes more like a model and sets its own properties based on sproc calls.

The code as I have written works, but my questions are about:

  1. GetReportDetails() - Is returning an instance of the parent class through a member method problematic for any reason? I know this is used in a Singleton, but this is not a singleton.

  2. The base class exposes only two properties, both of which are connection strings, since I am pulling data from two databases. Is there a better way to handle this?

  3. Any other feedback?

public class ReportDetails : ReportBase
{
    private readonly IConfiguration config;

    public int CustomReportId { get; private set; }
    public int FolderId { get; private set; }
    public string FolderName { get; private set; }
    public int? ReportTemplateId { get; private set; }
    public int PimsUserId { get; private set; }
    public string Name { get; private set; }
    public string Description { get; private set; }
    public bool Active { get; private set; }
    public string ProjectName { get; private set; }
    public int ProjectId { get; private set; }
    public List<AssociatedModule> AssociatedModules { get; private set; }
    public AssociatedModule SelectedAssociatedModule { get; private set; }
    public List<ReportTemplateName> TemplateNames { get; private set; }
    public ReportTemplateName SelectedTemplateName { get; private set; }
    public List<ReportFolder> ReportFolders { get; private set; }
    
    public ReportDetails(IConfiguration config) : base(config)
    {
        this.config = config;
    }

    public ReportDetails GetReportDetails(int customReportId, int? projectId, string projectName)
    {
        var inparams = new sproc_CustomReportGetDetailsModifiedVersion_Wrapper.In_Type();
        inparams.CustomReportId = customReportId;

        var sp = new sproc_CustomReportGetDetailsModifiedVersion_Wrapper(inparams);
        _ = DBClient.ExecOn(this.ProjectDBConnectionString).SpCall(sp);

        var outrecord = sp.Out.Records[0];

        Active = outrecord.Active;
        CustomReportId = outrecord.CustomReportId;
        Description = outrecord.Description;
        FolderId = outrecord.FolderId;
        FolderName = outrecord.FolderName;
        Name = outrecord.Name;
        PimsUserId = outrecord.PimsUserId;
        ReportTemplateId = outrecord.ReportTemplateId;
        ProjectName = projectName;
        ProjectId = Convert.ToInt32(projectId);
        AssociatedModules = GetAssociatedModules();
        SelectedAssociatedModule = GetSelectedModule();
        TemplateNames = GetTemplateNames();
        SelectedTemplateName = GetSelectedTemplateName();
        ReportFolders = GetReportFolders();
        return this;
    }

    private ReportTemplateName GetSelectedTemplateName()
    {
        //NOTE: TemplateNames uses "ReportId" as a synonym for ReportTemplateId.
        return TemplateNames.Single(e=>e.ReportId == ReportTemplateId);
    }

    private List<ReportTemplateName> GetTemplateNames()
    {
        return new ReportTemplateName(config).GetReportTemplateNames(SelectedAssociatedModule.AssociatedModuleId);
    }

     private AssociatedModule GetSelectedModule()
     {
        var inparams = new sproc_ReportDetailsGet_Wrapper.In_Type()
        {
            //NOTE: AssociatedModules uses "ReportID" as a synonym for ReportTemplateId.
            ReportId = ReportTemplateId,
        };

        var sp = new sproc_ReportDetailsGet_Wrapper(inparams);
        _ = DBClient.ExecOn(base.ReportDBConnectionString).SpCall(sp);

        int assModuleId = sp.Out.Records[0].AssociatedModuleId;
        return AssociatedModules.Single(e => e.AssociatedModuleId == assModuleId);
    }

    private List<AssociatedModule> GetAssociatedModules()
    {
        return new AssociatedModule(config).GetModuleList();
    }

    private List<ReportFolder> GetReportFolders()
    {
        return new ReportFolder(config).GetReportFolderList();
    }
}

The base class is very thin.

public abstract class ReportBase
    {
        public string ProjectDBConnectionString { get; private set; }
        public string ReportDBConnectionString { get; private set; }
        public ReportBase(IConfiguration config)
        {
            ProjectDBConnectionString = config.GetConnectionString("CLIENT_DB");
            ReportDBConnectionString = config.GetConnectionString("REPORTING");
        }

    }

Previously, a class may look something like this. Note there is no functionality - it is just a collection of properties, I think this is known as a Data Class, and according to the link above, it is generally considered a code smell:

public class ReportDetails
{
    public int CustomReportId { get;  set; }
    public int FolderId { get;  set; }
    public string FolderName { get;  set; }
    public int? ReportTemplateId { get;  set; }
    public int PimsUserId { get;  set; }
    public string Name { get;  set; }
    public string Description { get;  set; }
    public bool Active { get;  set; }
    public string ProjectName { get;  set; }
    public int ProjectId { get;  set; }
    public List<AssociatedModule> AssociatedModules { get;  set; }
    public AssociatedModule SelectedAssociatedModule { get;  set; }
    public List<ReportTemplateName> TemplateNames { get;  set; }
    public ReportTemplateName SelectedTemplateName { get;  set; }
    public List<ReportFolder> ReportFolders { get;  set; }
}

And there would be a manager that would operate on the data class, something like this:

    public class ReportDetailsManager
    {
        public List<ReportDetails> GetReportDetails()
        {
          // Build and return list of ReportDetails
        }

        public ReportDetails GetReportDetails(int reportDetailId)
        {
            ReportDetails reportDetails = new ReportDetails();
            // Go to the DB and assign properites 
            return reportDetails;
        }
    }

The crux of the refactor was to move the manager methods into what was formerly just a data class.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ maybe including the old pattern (that you've shifted from) would be helpful to give more helpful thoughts on how much you've gained, and what you can do better. Also, include the base class as well. \$\endgroup\$
    – iSR5
    Jan 8, 2021 at 1:56
  • \$\begingroup\$ sproc_CustomReportGetDetailsModifiedVersion_Wrapper.In_Type => this feels as if version control is done with class/method names. \$\endgroup\$
    – radarbob
    Nov 17, 2023 at 19:53

3 Answers 3

2
\$\begingroup\$

We commonly do it with "Meta-Information" hardcoded in the form of attributes.

This allows to derive all "Datamodels" from a baseclass, which then implements the databse interface in a generic way using the parameters stored in the attributes.

public class PropertyAttribute_Base : Attribute
    {
        /// <summary>
        /// The name of the procedure to execute
        /// </summary>
        public string spName;
        /// <summary>
        /// The name of the property holding the primary key
        /// </summary>
        public string pkName;
        /// <summary>
        /// The index of the Parameter for the specific procedure call
        /// </summary>
        public int ordPos;
        public PropertyAttribute_Base( string procName , string keyName , int ordinalPos ) { spName = procName; pkName = keyName; ordPos = ordinalPos; }
    }
    public sealed class PropertyAttribute_Load : PropertyAttribute_Base
    {
        public PropertyAttribute_Load( string procName , string keyName , int ordinalPos ) : base( procName , keyName , ordinalPos ) { }
    }

    public sealed class PropertyAttribute_Update : PropertyAttribute_Base
    {
        public PropertyAttribute_Update( string procName , string keyName , int ordinalPos ) : base( procName , keyName , ordinalPos ) { }
    }

    public sealed class PropertyAttribute_Store : PropertyAttribute_Base
    {
        public PropertyAttribute_Store( string procName , string keyName , int ordinalPos ) : base( procName , keyName , ordinalPos ) { }
    }

    public class DataModelBase
    {
        private string pk;

        private void LoadFromDatabase( string ConnectionString )
        {
            //Loop through all properties
            //and executed the stored procedure found
            //in the attribute together with the primary Key
        }

        public void UpdateInDataBase( string ConnectionString  )
        {
            //Loop through all properties
            //and executed the stored procedure found
            //in the attribute together with the primary Key
        }

        public void StoreToDataBase( string ConnectionString , string PrimaryKey = null )
        {
            if ( PrimaryKey != null ) pk = PrimaryKey;
            //Loop through all properties
            //and executed the stored procedure found
            //in the attribute together with the primary Key
        }

        public DataModelBase( string ConnectionString , string PrimaryKey ) { pk = PrimaryKey; LoadFromDatabase( ConnectionString ); }
    }

    public sealed class ObjectQueriedFromSingleDatabase : DataModelBase
    {
        [PropertyAttribute_Load( "ProcLoad" , null , 0 )]
        [PropertyAttribute_Update( "ProcUpdate" , null , 0 )]
        [PropertyAttribute_Store( "ProcStore " , null , 0 )]
        public int Property1 { get; set; }

        [PropertyAttribute_Load( "ProcLoad" , null , 1 )]
        [PropertyAttribute_Update( "ProcUpdate" , null , 1 )]
        [PropertyAttribute_Store( "ProcStore " , null , 1 )]
        public int Property2 { get; set; }

        [PropertyAttribute_Load( "ProcLoad" , null , 2 )]
        [PropertyAttribute_Update( "ProcUpdate" , null , 2 )]
        [PropertyAttribute_Store( "ProcStore " , null , 2 )]
        public int ListOfElementsKey { get; set; }

        [PropertyAttribute_Load( "ProcLoadList" , "ListOfElementsKey" , 0 )]
        [PropertyAttribute_Update( "ProcUpdateList" , "ListOfElementsKey" , 0 )]
        [PropertyAttribute_Store( "ProcStoreList" , "ListOfElementsKey" , 0 )]
        public List<string> ListOfElements { get; set; }

        public ObjectQueriedFromSingleDatabase( string ConnectionString , string PrimaryKey ) : base(ConnectionString, PrimaryKey ) { }
    }

You can get really fancy here and implement some of the following:

  • Partial loading/updating/storing via partial classes and class policy attributes
  • Order of procedure call execution for propertys within a class
  • Automated Exception Handling
  • Consistency/"Does the value make sense?" checks for attributes
  • Objects stored accross multiple databases via partial classes or sub classes

And a nice feature (once you implemented the "complex" base-class and its logic) is:

  • You can auto-generate the user-objects pretty simple from a procedure definition script
  • The other way around works as well...! Also table definition scripts can be auto generated quite easy!
  • No more implementing custom code ... just configure the attributes
  • Did i just say, that the intern can "code" all the objects in the future?
  • Or did you hear, that exporting the procedure definitions of your database and using a simple interpreter will yield you all "DataObjectXYZ.cs" with the click of a button in the future?
\$\endgroup\$
1
\$\begingroup\$

I don't see any code smell in the previous version as data model. The provided article seems to consider data models code smell because it's not enforcing OOP principles, which I understand. However, in real-world code scenarios, data models are very useful and widely used in all programming languages not only in C#. Without them, it will be hard to work with DBMS (database management system), flatten objects, and many other useful scenarios where using data models would add more maintainability to the objects.

You can take a look on .NET reference code, you will find many data class models that have been used under the hood, and they are maintained by either parent class or another classes.

In the end, I would say that the previous version is much better in handling than the new one.

\$\endgroup\$
1
\$\begingroup\$

You are committing the same "code smell" sin with public abstract class ReportBase. All properties and no functionality.

The abstract class has no polymorphism provisions for subclassing. All the ReportDetails methods should be in ReportBase. That done, then there seems to be no need for polymorphic behavior so you don't need a base class of any kind.

Parameterizing the "data object" makes no sense without associated parameterized SQLAdapter objects. That would be a solid basis for subclassing. Otherwise the data can be injected with a method. But again, there is no need to subclass in this code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.