Loading a Model from a Database

As you will see from the code below, I have a base model.

```
public class BasePDV
{
public int CaseID { get; set; }
public string CustContactTel { get; set; }
public string CustMobileTel { get; set; }
public string PropertyType { get; set; }
public string PremisesStatus { get; set; }
public int LockType { get; set; }
public int AccessInfo { get; set; }
public string V1 { get; set; }
public string V2 { get; set; }
public int V2a { get; set; }
public string V3 { get; set; }
public string V4 { get; set; }
public string V4a { get; set; }
public string V5 { get; set; }
public string V6 { get; set; }
public string V7 { get; set; }
public string V7a { get; set; }
public string V8 { get; set; }
public string V8a { get; set; }
public string V9 { get; set; }
public string V9a { get; set; }
public string V10 { get; set; }
public int V10a { get; set; }
public string V11 { get; set; }
public string V11a { get; set; }
public string V12 { get; set; }
public string VulnerableOther { get; set; }
public string ContactType { get; set; }
public string NINumber { get; set; }
public string ContactDate { get; set; }
public string CustDOB { get; set; }
public double? PayAmount { get; set; }
public double? PayAmount2 { get; set; }
public int PayMethod { get; set; }
public string ReceiptNo { get; set; }
public double? DirectPayAmount { get; set; }
public bool COT { get; set; }
public string TitleCode { get; set; }
public string OtherTitle { get; set; }
public string FirstName { get; set; }
public string MiddleInitials { get; set; }
public string SurName { get; set; }
public string COTProofType { get; set; }
public string COTDate { get; set; }
public string PropRespMtrRead { get; set; }
public string PropRespMtrBRead { get; set; }
public string PrevAdd1 { get; set; }
public string PrevAdd2 { get; set;

During our discussion in the comments, it became clear to me that you're mixing the business objects with how they're displayed. How they're displayed has little to do with how you model your business objects. Or, at least it should. You hear the word "model" tossed around a lot, but in reality, there are two different kinds of models. There are business models, and view models. Many times these are identical, sometimes they're not. In this case, I think it's clear that the business model and the view model are almost, but not quite, the same.

Right now, the business wants all of this data in what amounts to a spreadsheet, but what happens when they see the beta version and decide they were wrong and would actually like separate detail screens after all? Then you would have no business or view models to represent these subsets of data. Note that I do not believe this to be a YAGNI situation, because it's very likely that this could happen. If not at beta time, perhaps 6 months from now. Either way, it's likely that you'll need to access these business objects on their own at some point in the future. Set yourself up for success. Minimally, this work will help you to clarify your code.

I imagine your PDV class looking something like this.

public class PDV
{
    DateTime TimeStamp {get; set;}
    PropertyInfo PropertyDetails {get; set;}
    VunerabilityInfo VunerabilityDetails {get; set;}
    ContactInfo Contact {get; set;}
    PaymentInfo PaymentDetails {get; set;}

    //... so and so on
    // basically one for each of the comments in `LoadPDV`
}

Now, you still actually load all of this from the same view, and still all at once (although, now you have the option of going to the database for any one of these objects!). Now, you just create each of these objects from the view data and add it to the PDV.

using (SqlDataReader dr = cmd.ExecuteReader())
{
    while (dr.Read())
    {
        pdv.Timestamp = dr["PDVStamp"];

        var property = new PropertyInfo()
        {
            property.CustContactTel = dr["CustContactTel"].ToString();
            property.CustMobileTel = dr["CustMobileTel"].ToString();
            property.PropertyType = dr["PropertyType"].ToString();
            property.PremisesStatus = dr["PremisesStatus"].ToString();
            property.LockType = string.IsNullOrEmpty(dr["LockType"].ToString()) ? 0 : Convert.ToInt16(dr["LockType"]);
            property.AccessInfo = string.IsNullOrEmpty(dr["AccessInfo"].ToString()) ? 0 : Convert.ToInt16(dr["AccessInfo"]);
        }

        pdv.PropertyDetails = property;

But that's more code! Yes. It is, but we're not going to leave it this way. We're going to extract methods that take in the data reader and return new business objects.

using (SqlDataReader dr = cmd.ExecuteReader())
{
    while (dr.Read())
    {
        pdv.Timestamp = dr["PDVStamp"];
        pdv.PropertyDetails = CreatePropertyDetails(dr);
        pdv.VunerabilityDetails = CreateVunerabilityDetails(dr);

        //...

Now, there's a trade off to be had here. We now have more sensible objects and code. You have the ability to quickly create new views based on these other business objects and the ability to quickly create different ways of getting them back from the database. That's good. The bad part of this is that you'll have to explicitly bind each field to a column in the DataGridView. You know what though, that's not really all that bad. Explicit binding is better in my opinion anyway. Yeah, it's nice to let Visual Studio automatically bind an object to a gridview, but I've been bitten more than once by changing a class's interface, only to have one of my forms suddenly "broken" and not displaying information the same way. Had I explicitly bound it to begin with, it wouldn't have been an issue when I changed the class.

One last thing...


The method GetCommandParameters is used for saving the data

A method that starts with Get should never be saving anything. No side effects. If I call GetFoo, I expect it to retrieve me a Foo, not save that foo to the database.

StackExchange Code Review Q#92765, answer score: 2