3
\$\begingroup\$

I just started out a new job as a C# programmer on .NET 3.5. My manager gives me total independence on how I build my modules, which is generally a good thing, but there are minuses too. Having too much "freedom" tends to make me wander to places you wouldn't have if you had some strict guidance.

My project involves many simple forms and reports, usually one table for each form, sometimes I have a 1-to-1 or 1-to-Many relations on a specific form. Also, my business objects are not complicated for now, every table on the database maps to on exactly one business object.

So, I did my research and decided that for my DAL which isn't too complicated I'll be using a Linq-to-SQL within simple Repositories classes as it provides me with a simple ORM mappings of objects. Also. in .NET 3.5, I felt it's my best option (you are more than welcome to correct me if I'm wrong i.e if you can achieve ORM advantages with vanilla ADO.NET).

I then decided that I'll be using an MVP pattern to tie up my layers, giving me a well de-coupled UI/Business Logic. Having said all that, theory worked perfect but soon as I started with my first module many questions arise. I don't want to make this question too general so I'll give a real life example and explain my confusions once at a time, trying to keep it as concise as I can.

Simple module example:

I have 2 forms, Main form which presents a Person details, this form has a button that when you click a sub-form will popup with that person Children.

Table Person: id(PK),name
Table Children: id(PK),name,parent

Corresponding to those tables I have the following model classes:

Person Class

[Table(Name = "Person")]
public class Person
{
    public Person()
    {
        _years = new EntitySet<PersonYear>(OnYearAdded, OnYearRemoved);
        _children = new EntitySet<PersonChildren>(OnChildrenAdded, OnChildrenRemoved);
    }

    [Column(Name = "ID", IsPrimaryKey = true, IsDbGenerated = true, CanBeNull = false)]
    public int PersonID { get; set; }
    [Column(Name = "NAME")]
    public string FullName { get; set; }
    [Column(Name = "PERS_FIRST_NAME")]
    public string FirstName { get; set; }
    [Column(Name = "PERS_LAST_NAME")]
    public string LastName { get; set; }
    [Column(Name = "PERS_GENDER", CanBeNull = true)]
    public byte? Gender { get; set; }
    [Column(Name = "PERS_IDENTITY_NUM", CanBeNull = true)]
    public int? IdentityNumber { get; set; }
    [Column(Name = "PERS_BIRTH_DATE", CanBeNull = true)]
    public DateTime? BirthDate { get; set; }
    [Column(Name = "CODE", CanBeNull = true)]
    public long? PersonCode { get; set; }
    [Column(Name = "IS_COMPANY")]
    public bool IsCompany { get; set; }
    [Column(Name = "ADR_CITY")]
    public string City { get; set; }
    [Column(Name = "ADR_STREET")]
    public string Street { get; set; }
    [Column(Name = "ADR_ZIP")]
    public string Zip { get; set; }
    [Column(Name = "CODE_RAMPLUS")]
    public int? BusinessCode { get; set; }
    [Column(Name = "CCR_NMB")]
    public string TaxFileNumber { get; set; }

    //Partner Details

    [Column(Name = "PERS2_GENDER", CanBeNull = true)]
    public byte? PartnerGender { get; set; }
    [Column(Name = "PERS2_IDENTITY_NUM", CanBeNull = true)]
    public int? PartnerIdentityNumber { get; set; }
    [Column(Name = "PERS2_BIRTH_DATE", CanBeNull = true)]
    public DateTime? PartnerBirthDate { get; set; }


    private EntitySet<PersonChildren> _children;

    [Association(Storage = "_children", OtherKey = "PersonID")]
    public EntitySet<PersonChildren> Children
    {
        get { return this._children; }
        set { this._children.Assign(value); }
    }

    private EntitySet<PersonYear> _years;

    [Association(Storage = "_years", ThisKey = "PersonID", OtherKey = "PersonID")]
    public EntitySet<PersonYear> Years
    {
        get { return this._years; }
        set { this._years.Assign(value); }
    }

    private void OnYearAdded(PersonYear addedYear)
    {
        addedYear.Person = this;
    }

    private void OnYearRemoved(PersonYear removedYear)
    {
        removedYear.Person = null;
    }

    private void OnChildrenAdded(PersonChildren addedChildren)
    {
        addedChildren.Person = this;
    }

    private void OnChildrenRemoved(PersonChildren removedChildren)
    {
        removedChildren.Person = null;
    }

}

PersonChildren Class

[Table(Name="dbo.PersonChildren")]
public class PersonChildren
{
    [Column(Name="CHL_ID",IsPrimaryKey=true,IsDbGenerated=true)]
    public int ChildrenID { get; set; }
    [Column(Name="CHL_Person_ID")]
    public int? PersonID { get; set; }
    [Column(Name="CHL_NAME")]
    public string Name { get; set; }
    [Column(Name="CHL_BIRTHDATE")]
    public DateTime? BirthDate { get; set; }
    [Column(Name="CHL_CUSTODY")]
    public int? Custody { get; set; }
    [Column(Name="CHL_CUSTODY_BENEFIT")]
    public bool? CustodyBenefit { get; set; }
    [Column(Name="CHL_CUSTODY_NO_BENEFIT")]
    public bool? CustodyNoBenefit { get; set; }
    [Column(Name="CHL_SHARED_CUSTODY")]
    public bool? SharedCustody { get; set; }
    [Column(Name="CHL_SINGLE_PARENT")]
    public bool? IsSingleParent { get; set; }

    EntityRef<Person> _Person;
    [Association(Storage = "_Person", ThisKey = "PersonID", OtherKey = "PersonID", IsForeignKey = true)]
    public Person Person
    {
        get { return _Person.Entity; }
        set { _Person.Entity = value; }
    }
}

To get Data from my models I have created a class PersonRepository. This class is in charge of querying the Database and fetch all the data that the PersonView needs, all done by Linq-to-SQL.

PersonRepository Class

public class PersonRepository
{
    private readonly MainDBDataContext myDB;

    public PersonRepository()
    {
        myDB = new MainDBDataContext();
    }

    public IList<PersonDataPerYear> AllPerYear(int iYear)
    {
        var Person = (from c in myDB.Person
                       join cy in myDB.PersonYear on c.PersonID equals cy.PersonID
                       where !c.IsCompany && cy.Year == iYear
                       select new PersonDataPerYear
                       {
                           BirthDate = c.BirthDate,
                           PersonID = c.PersonID,
                           FirstName = c.FirstName,
                           LastName = c.LastName,
                           IdentityNumber = c.IdentityNumber,
                           Gender = c.Gender,
                           PersonCode = c.PersonCode,
                           Year = cy.Year,
                           YearID = cy.YearID
                       }).ToList();
        return Person;
    }

    public Person GetTaxPayerByID(int taxPayerID, int iYear)
    {
        DataLoadOptions dlo = new DataLoadOptions();
        dlo.AssociateWith<Person>(
            c => c.Years.Where(y => y.Year == iYear));
        dlo.LoadWith<Person>(c => c.Years);
        myDB.LoadOptions = dlo;

        var Person = (from c in myDB.Person
                      where c.PersonID == taxPayerID && !c.IsCompany
                      select c).SingleOrDefault();
        return Person;
    }

    public void Update(IList<PersonDataPerYear> Person)
    {
        foreach (var person in Person)
        {
            var original = myDB.Person.First(c => c.PersonID == person.PersonID);
            original.FirstName = Person.FirstName;
            original.LastName = Person.LastName;
            myDB.SubmitChanges();
        }
    }

    public void Insert(Person Person)
    {
        myDB.Person.InsertOnSubmit(Person);
        myDB.SubmitChanges();
    }


    public long? GetNextPersonCode()
    {
        long? result = myDB.Person.DefaultIfEmpty().Where(c => c != null).Max(c => c.PersonCode);
        result = result ?? 1;
        if (result > 0)
            return result;
        else
            return 1;
    }


    public void SaveChanges()
    {
        myDB.SubmitChanges();
    }


}

The MVP implemented in a passive way so I have an interface IPersonView and an actual View that implements it named PersonView. The PersonView is responsible on validating itself.

IPersonView

public interface IPersonView
{
    event Action Save;
    event Action MartialStateChange;
    event Action ChildrenClicked;

    void LoadPerson(Person Person);
    bool MartialPanelVisable { get; set; }
    bool IsMarried { get;  }
    int PersonChildrenCount { get; set; }
}

PersonView

public partial class PersonView : Form , IPersonsView
{
    public event Action Save;
    public event Action MartialStateChange;
    public event Action ChildrenClicked;

    public PersonView()
    {
        InitializeComponent();
        InitLookups();
    }

    public bool IsMarried { get { return AppUtils.ObjToInt(luMartialStatus.EditValue) == 1; } }

    public void LoadPerson(Person data) 
    {
        var yearData = data.Years.First<PersonYear>();

        edtPersonCode.DataBindings.Add("Text",data , "PersonCode", true);
        edtFirstName.DataBindings.Add("Text", data, "FirstName", true);
        edtLastName.DataBindings.Add("Text", data, "LastName", true);
        edtIdentityNumber.DataBindings.Add("Text", data, "IdentityNumber", true);
        edtSreet.DataBindings.Add("Text", data, "Street", true);
        edtCity.DataBindings.Add("Text", data, "City", true);
        edtZip.DataBindings.Add("Text", data, "Zip", true);
        deBirthDate.DataBindings.Add("DateTime", data, "BirthDate", true);
        deBirthDate2.DataBindings.Add("DateTime", data, "PartnerBirthDate", true);
        edtLastName2.DataBindings.Add("Text", yearData, "PartnerLastName", true);
        edtFirstName2.DataBindings.Add("Text", yearData, "PartnerFirstName", true);
        edtIdentityNumber2.DataBindings.Add("Text", data, "PartnerIdentityNumber", true);
        edtTaxFileNumber.DataBindings.Add("Text", data, "TaxFileNumber", true);

        luTaxClerk.DataBindings.Add("EditValue", yearData, "TaxClerk", true);
        luPaymentDoc.DataBindings.Add("EditValue", yearData, "PaymentDoc", true);
        luMartialStatus.DataBindings.Add("EditValue", yearData, "MartialState", true);
        luGender.DataBindings.Add("EditValue", data, "Gender", true);
        luGender2.DataBindings.Add("EditValue", data, "PartnerGender", true); 
    }


     private void InitLookups()
    {
        DataTable dt = new DataTable();
        DataColumn id = new DataColumn("id",typeof(int));
        DataColumn desc = new DataColumn("desc",typeof(string));
        dt.Columns.AddRange( new [] {id , desc});
        dt.Rows.Add(0,"none");
        dt.Rows.Add(1,@"monthly");
        dt.Rows.Add(2,@"dual");
        luPaymentDoc.Properties.DataSource = dt;
        luPaymentDoc.Properties.ValueMember = "id";
        luPaymentDoc.Properties.DisplayMember = "desc";

        DataTable dt2 = new DataTable();
        DataColumn id2 = new DataColumn("id", typeof(int));
        DataColumn desc2 = new DataColumn("desc", typeof(string));
        dt2.Columns.AddRange(new[] { id2, desc2 });
        dt2.Rows.Add(1, "male");
        dt2.Rows.Add(2, "female");
        luGender.Properties.DataSource = dt2;
        luGender.Properties.ValueMember = "id";
        luGender.Properties.DisplayMember = "desc";

        luGender2.Properties.DataSource = dt2;
        luGender2.Properties.ValueMember = "id";
        luGender2.Properties.DisplayMember = "desc";

        luTaxClerk.Properties.DataSource = RamDataSet.GetMinistries();
        luTaxClerk.Properties.DisplayMember = "MNS_Description";
        luTaxClerk.Properties.ValueMember = "MNS_ID";

        luMartialStatus.Properties.DataSource = RamDataSet.GetPersonMartialStatuses();
        luMartialStatus.Properties.DisplayMember = "CMS_DESCRIPTION";
        luMartialStatus.Properties.ValueMember = "CMS_STATUS_ID";


    }

     private bool ValidateUserInput()
     {
         ResetReqLablesError();

         if (edtLastName.Text.Trim().Length == 0)
         {
             edtLastName.Focus();
             lbReqFamilyName.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (edtFirstName.Text.Trim().Length == 0)
         {
             edtFirstName.Focus();
             lbReqFirstName.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (edtIdentityNumber.Text.Trim().Length == 0) // Check if there is ID Validation somewhere.
         {
             edtIdentityNumber.Focus();
             lbReqIdentityNumber.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (IDValidator.ISValidId(edtIdentityNumber.Text) == -1)
         {
             edtIdentityNumber.Focus();
             lbReqIdentityNumber.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (edtSreet.Text.Trim().Length == 0)
         {
             edtSreet.Focus();
             lbReqSreet.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (edtCity.Text.Trim().Length == 0)
         {
             edtCity.Focus();
             lbReqCity.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         if (edtZip.Text.Trim().Length == 0)
         {
             edtZip.Focus();
             lbReqZip.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         DateTime temp;
         if (!DateTime.TryParse(deBirthDate.Text, out temp))
         {
             deBirthDate.Focus();
             lbReqBirthDate.Visible = true;
             MessageUtils.ShowError("error!");
             return false;
         }

         // Validate partner details
         if (IsMarried)
         {
             if (edtLastName2.Text.Trim().Length == 0)
             {
                 edtLastName2.Focus();
                 lbReqLastName2.Visible = true;
                 MessageUtils.ShowError("error!");
                 return false;
             }

             if (edtFirstName2.Text.Trim().Length == 0)
             {
                 edtFirstName2.Focus();
                 lbReqFirstName2.Visible = true;
                 MessageUtils.ShowError("error!");
                 return false;
             }

             if (edtIdentityNumber2.Text.Trim().Length == 0) // Check if there is ID Validation somewhere.
             {
                 edtIdentityNumber2.Focus();
                 lbReqIdentityNumber2.Visible = true;
                 MessageUtils.ShowError("error!");
                 return false;
             }

             if (IDValidator.ISValidId(edtIdentityNumber2.Text) == -1)
             {
                 edtIdentityNumber2.Focus();
                 lbReqIdentityNumber2.Visible = true;
                 MessageUtils.ShowError("error!");
                 return false;
             }

             DateTime temp2;
             if (!DateTime.TryParse(deBirthDate2.Text, out temp2))
             {
                 deBirthDate2.Focus();
                 lbReqBirthDate2.Visible = true;
                 MessageUtils.ShowError("error!");
                 return false;
             }

         }

         return true;
     }

     private void ResetReqLablesError()
     {
         lbReqCity.Visible = lbReqFamilyName.Visible = lbReqFirstName.Visible = lbReqIdentityNumber.Visible = lbReqSreet.Visible = lbReqZip.Visible =
         lbReqFirstName2.Visible = lbReqLastName2.Visible = lbReqIdentityNumber2.Visible = lbReqBirthDate2.Visible = false;
     }

    private void btnSave_Click(object sender, EventArgs e)
    {
        SaveAndExit();
    }

    private void btnCancel_Click(object sender, EventArgs e)
    {
        this.DialogResult = DialogResult.Cancel;
    }

    private void PersonView_KeyDown(object sender, KeyEventArgs e)
    {
        switch (e.KeyCode)
        {
            case Keys.F10:
                btnSave.PerformClick();
                e.Handled = true;
                break;
        }
    }

    private void SaveAndExit()
    {
        if (ValidateUserInput())
        {
            if (Save != null)
            {
                this.Save();
            }

            this.DialogResult = DialogResult.OK;
        }
    }

    private void luMartialStatus_EditValueChanged(object sender, EventArgs e)
    {
        if (MartialStateChange != null)
        {
            this.MartialStateChange();
        }
    }

    public bool MartialPanelVisable
    {
        get { return pnlPartner.Visible;}
        set { this.pnlPartner.Visible = value; }
    }

    private void btnChildrens_Click(object sender, EventArgs e)
    {
        if (ChildrenClicked != null)
        {
            ChildrenClicked();
        }
    }

    public int PersonChildrenCount { get { return Int32.Parse(this.edtChildrenCount.Text); } set { this.edtChildrenCount.Text = value.ToString(); } }

}

Here is the PersonPresenter, It is the glue between the IPersonView and the Model. It is currently only responsible for the Main-Form (Person).

PersonPresenter Class

class PersonPresenter
{
    private readonly PersonRepository personsRepository;
    private readonly IPersonsView personView;
    private readonly bool createMode;
    private readonly int personID;
    private Person person;

    public PersonPresenter(PersonRepository personsRepository, IPersonsView personView, int iYear, bool createMode, int personID )
    {
       this.personsRepository = personsRepository;
       this.personView = personView;
       this.createMode = createMode;
       this.personID = personID;

       personView.Save +=new Action(OnSaveButtonClick);
       personView.MartialStateChange += new Action(OnMartialStateChange);
       personView.ChildrenClicked += new Action(OnChildrenButtonClicked);

        if( !createMode )
        {
            person = personsRepository.GetPersonByID(this.personID,iYear);
            if (person.Years.Count == 0)
            {
                var year = new PersonYear();
                year.Year = iYear;
                person.Years.Add(year);
            }
        }
        else 
        {
            person = new Person();
            person.PersonCode = personsRepository.GetNextPersonCode();
            var year = new PersonYear();
            year.Year = iYear;
            person.Years.Add(year);
        }

        if (person != null)
        {
            personView.LoadPerson(person);
            personView.PersonChildrenCount = person.Children.Count;
        }               
    }

    void OnSaveButtonClick()
    {
        person.FullName = person.FirstName + " " + person.LastName;
        if (!createMode)
        {
            personsRepository.SaveChanges();
        }
        else 
        {
            personsRepository.Insert(person);
        }
    }

    void OnMartialStateChange()
    {
        if (personView.IsMarried)
        {
            personView.MartialPanelVisable = true;
        }
        else 
        {
            personView.MartialPanelVisable = false;
        }
    }

    void OnChildrenButtonClicked() 
    { 
        PersonChildrenView view = new PersonChildrenView();
        PersonChildrenPresenter pcp = new PersonChildrenPresenter(personsRepository, view, person);
        view.ShowDialog();

        personView.PersonChildrenCount = person.Children.Count;


    }
}

Views and Presenter for the sub-form:

IPersonChildrenView Inerface

public interface IPersonChildrenView
{
    event Action Save;

    void LoadChildren(BindingList<PersonChildren> children);

}

PersonChildrenPresentor

public class PersonChildrenPresenter
{
    private IPersonChildrenView view;
    private PersonRepository repository;
    private Person person;
    private BindingList<PersonChildren> localChildren;
    private Dictionary<PersonChildren,PersonChildren> originalChildren;

    public PersonChildrenPresenter(PersonRepository personRepository, IPersonChildrenView view, Person person)
    {
        this.view = view;
        this.repository = personRepository;
        this.person = person;

        view.Save += new Action(OnSaveButtonClicked);

        originalChildren = new Dictionary<PersonChildren, PersonChildren>();
        this.CopyChildren();
        view.LoadChildren(localChildren);
    }

    public void OnSaveButtonClicked()
    {
        foreach (var child in localChildren)
        {
            // children is from original list
            if (originalChildren.ContainsKey(child))
            {
                {
                    var original = originalChildren[child];
                    if (original != null)
                    {
                        original.BirthDate = child.BirthDate;
                        original.ChildrenID = child.ChildrenID;
                        original.Person = child.Person;
                        original.PersonID = child.PersonID;
                        original.Custody = child.Custody;
                        original.CustodyBenefit = child.CustodyBenefit;
                        original.CustodyNoBenefit = child.CustodyNoBenefit;
                        original.IsSingleParent = child.IsSingleParent;
                        original.Name = child.Name;
                        original.SharedCustody = child.SharedCustody;
                    }
                }
            }
            // children is added
            else
            {
                person.Children.Add(child);
            }
        }

        // children deleted - cointained in dictionary but not in copied list.
        var deleted = originalChildren.Keys.Where(key => !localChildren.Contains(key)).ToList();
        foreach (var deletedChild in deleted)
        {

            person.Children.Remove(originalChildren[deletedChild]);               
        }


    }

    public void CopyChildren()
    {
        var temp = (person.Children.Select(c=> new { Original = c , Copy= new PersonChildren
        {
            Person = c.Person,
            BirthDate = c.BirthDate,
            ChildrenID = c.ChildrenID,
            PersonID = c.PersonID,
            Custody = c.Custody,
            CustodyBenefit = c.CustodyBenefit,
            CustodyNoBenefit = c.CustodyNoBenefit,
            IsSingleParent = c.IsSingleParent,
            Name = c.Name,
            SharedCustody = c.SharedCustody
        }}

        )).ToList();

        foreach (var child in temp)
        {
            originalChildren.Add(child.Copy,child.Original);           
        }

        localChildren = new BindingList<PersonChildren>(temp.Select(c=>c.Copy).ToList());
    }
}

Invocation of the code:

PersonView view = new PersonView();
PersonRepository repo = new PersonRepository();
PersonPresenter presenter = new PersonPresenter(repo, view, Year, true, 0);     
view.ShowDialog();

Here are the things I'm struggling with.

First with the Repository class:

  • How many of this repositories should I have, for each Table? Entity? Form? set for Forms? Is there any convention on that part?
  • The second form needs the Person Children details. Does that means it'll get a copy of the same PersonRepository object?
  • Is it correct to create an online context that lives throughout the lifetime of the Repository? What are the trade-offs, when should I use the disconnected approach?

Here is my PersonPresenter constructor:

public PersonPresenter { IPersonView view, PersonRepository repo }

Here is my ChildrenPresenter constructor:

public ChildrenPresenter { IChildrenView view, PersonRepository repo }

As you can see I'm using same PersonRepository on the ChildrenPresenter.

Here are more questions I seem to wonder about:

  • In simple cases like this should I just use one presenter for both forms? Or is it better to have one Presenter per form?
  • What if a form suddenly introduces a new Entity data to be taken from another table which belongs on a different Repository? What should I do then to get that data? As I said at the moment I don't have this case but I just wonder.
  • Let's say that now I have a report that needs the Person and Children data. Should it also use the same PersonRepository which means that from now on it's the central Repository for a Person?

As you can see, I am pretty much bedazzled by all the paths I can take. I know that it's all a matter of experience and time but I do want to accelerate my knowledge. I know my DAL implementation is missing out on Transactions and that Repositories uses interfaces, but I have no idea how should I go about it.

\$\endgroup\$
  • 1
    \$\begingroup\$ Great, there you go I added most of the code minus some of the discrete business stuff. Hope that now the picture of what I have so far is clear. \$\endgroup\$ – yossi elimelech May 23 '15 at 18:53
  • \$\begingroup\$ Why all the prefixes in the table fields, like "ADR_" or "CHL_"? Why not simply have those fields be named the same as the names in the code? Why name the field "CCR_NMB" in the table, but "TaxFileNumber" in the code? Is this a requirement or something of your choosing? \$\endgroup\$ – BCdotWEB May 28 '15 at 6:22
  • \$\begingroup\$ Hey @BCdotWEB, I got the database tables as is from a working product that uses it. Renaming would require some serious overhead for multiple clients. So at least in my code I made the fields more readable. \$\endgroup\$ – yossi elimelech May 28 '15 at 6:35
  • \$\begingroup\$ You should include that information at the top. I got the impression you had total freedom, but you're limited by existing db design. That's important information WRT a code review. Hence my mostly obsolete review... \$\endgroup\$ – BCdotWEB May 28 '15 at 6:49
  • \$\begingroup\$ Also: why do you use WinForms, which is basically outdated, and not WPF? Is that a requirement, or something you chose? \$\endgroup\$ – BCdotWEB May 28 '15 at 7:25
2
\$\begingroup\$

Not going to review all that, just going to focus on one aspect: your table structure.

Edit: ugh, now I read your comment (which was posted while I was doing this review) and most of my review is irrelevant. Still, my point remains: this is really bad database design.


Person

  • Looking at the Person class I have to wonder: what if someone moves?

  • Having both FullName and the fields FirstName and LastName is just asking for trouble.

  • Why name a field PersonCode? You're already in the table Person, so we know this field Code is related to the Person. Just name it Code.


PersonChildren

  • PersonChildren is a bad name for a class, since it is a plural. This class will describe one child, not multiple.

  • Along those lines: PersonID is an "unnatural" name for a field I'd expect to be named ParentID.

  • Moreover is a Child not a person? Isn't there a case possible where a Child will also be a Person, i.e. someone who'll receive custody benefits but also has to pay them?

  • Wouldn't it make more sense to have a Person class, and then both a Parent and a Child class which link to that class for data like name and date of birth?


Some small remarks:

\$\endgroup\$
  • \$\begingroup\$ You are correct the database design is awful. I can't argue with that ;) As I said I inherent it and I don't really have much options. Took your advise on the code aspects. BTW, if you think it's weird that Person and Children are in different tables, you'll be in shock that a Person's spouse is also in different table... oh lord, such a mess :( \$\endgroup\$ – yossi elimelech May 28 '15 at 7:00
0
\$\begingroup\$

Your repository class has a Disposable member, so it should implement IDisposable.

public class PersonRepository
{
    private readonly MainDBDataContext myDB;

But I personally don't like doing that if I can help it. You can remove the need to implement the interface if you keep your connections short lived.

using (var dbContext = new MainDbContext())
{
    // do database stuff
}

It's not very DRY or configurable this way though, so I tend to go one step further, and make my repositories look something like this.

public class LinqToSqlPersonRepository : IPersonRepository
{
    private readonly Func<MainDbContext> _dbContextFactory;

    public LinqToSqlPersonRepository(string connectionString)
    {
        _dbContextFactory = ()=> new MainDbContext(connectionString);
    }

    public GetPerson(int id)
    {
        using (var dbContext = _dbContextFactory.Invoke();
        {
            // do Db stuff
        }
    }

This will make it much easier to pass your database connection in so it's trivial to deploy your code to different environments.


I'm not a fan of putting all that validation logic inside your view. What happens when we need a second, slightly different person view? Or we need a Person with the same validation logic in our shinny new WPF app? We're going to copy/paste that logic because it can't be shared with this architecture. There's a reason that newer Windows platforms support DataAnnotations and the concept that the model should know how to validate itself. You could tackle it several ways, but any route you take will end up with a new IPersonView.ShowValidationError() method.

You could implement a separate PersonValidator, roll your own IValidatable interface, or go way over the top and create your own DataAnnotations framework for Winforms that leverages the same IValidatableObject interface that MVC and WPF use, but you'll eventually regret that "passive presenter" if you don't get that logic out of the code behind.

Note: Letting the model validate itself is still a passive presenter, just a (maybe) more correct implementation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.