4
\$\begingroup\$

If I have a Person class that outlines the properties of a person and one of People that just contains a List<Person> like so:

public class Person
{
    public Person(){}
    public int personID {get;set;}
    public string name {get;set;}
    public string address {get;set;}
}

public class People
{
    public People(){}

    public List<Person> peoples {get;set;}

    public List<Person> getPeople()
    {
        List<Person> p = new List<Person>();
        //create database connection
        //open datareader

        Person onePerson = new Person();
        //write data to eahc property type and add data to list

        p.Add(onePerson);
        return p;
    }
}

To call the database method that gets the people from the database I could do this:

static void main(string[] args)
{
    People p = new People();
    p.getPeople();
}

Is this how I would do this in terms of OOP?

I'm just beginning to use these techniques in my coding as usually I would remove the getPeople() method from the People class and do this in main():

static void main(string[] args)
{
    List<Person> people = getPeople();
}

private static List<Person> getPeople()
{
    List<Person> p = new List<Person>();
    //create database connection
    //open datareader
    //load data into list

    Person onePerson = new Person();

    p.Add(onePerson);
    return p;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm not seeing the point of those empty public constructors... \$\endgroup\$ – Jesse C. Slicer Nov 5 '13 at 0:52
  • 1
    \$\begingroup\$ peoples is a rather confusing name for a List<Person>, especially in a class called People. \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 1:47
  • 1
    \$\begingroup\$ Since it's a code review site: your code is tagged as C# and looks like Java. Method names should start with upper case, same goes for properties. \$\endgroup\$ – Migol Nov 5 '13 at 18:59
  • \$\begingroup\$ @Migol YESSS!!! I'm not the only one!! (been biting my tongue on this one) \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 19:13
5
\$\begingroup\$

Very few things in life are free. C# default constructors are one of those, the compiler generates one automatically for you if your class doesn't explicitly have one! So unless you have another constructor (with parameters) and you also need a parameterless one, putting it in is just clutter.

Encapsulating database operations in a dedicated object (like you're doing with your People class), vs doing it all in a static method, is a step forward in the right direction.. but I don't see what the peoples property is trying to encapsulate.

Think of classes as abstractions - you have a Person abstraction which represents a single record in your database. Then you have a People abstraction which represents an object that can manipulate Person records. That's all nice and tidy and high-level - you don't want to mix this niceness with boilerplate-level SQL connections and queries. Doing that is writing code at the wrong level of abstraction.

So the next step forward would be to create another class that's responsible for dealing with these things - a type that would deal with setting up a connection, running a query and then disposing the connection and every other IDiposable along the way.

Once you've done that, you will (hopefully) realize you've just reinvented the wheel - and that your wheel is more than likely square. At this stage it's all good, the worst thing you could do is start with an Entity Data Model that does it all for you: you want to understand what's going on, if it feels automagic then you've skipped something.

So first (well, after you've written your square wheel!) take a look at Linq to SQL and see what a DataContext can do for you.

\$\endgroup\$
  • \$\begingroup\$ So I should have the getPeople() defined in the People class? \$\endgroup\$ – BS123 Nov 5 '13 at 3:10
  • \$\begingroup\$ I see your People class as some sort of Person repository whose job is to perform data operations on Person, so yes - and then you'll want to add some AddPerson, RemovePerson and UpdatePerson methods, too - they all belong in there :) \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 3:15
  • \$\begingroup\$ But the People.peoples property is wrong though; if the job of People is to perform data operations, keeping a copy of the records is breaking this single responsibility - and I'm not even mentioning the public setter! \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 3:21
1
\$\begingroup\$

Personally, I think the better approach would be this:

public class Person
{
    public Person(){}
    public int personID {get;set;}
    public string name {get;set;}
    public string address {get;set;}

    public static List<Person> GetPeople() { }
}

Now to get a list of Person objects, you just do this:

var people = Person.GetPeople();

And just get rid of the People class. It isn't really doing anything. In fact, you're just building a new data structure when the List<T> is perfect already.


Alright, in light of the comments on this post, let me dig a little deeper for you on this one.

I believe the approach I'd take goes much further than the aforementioned simplified solution. So, let's get started. The first thing I'd do is include Dapper. This is available via NuGet.

The next thing I'd do is build a static method to wrap the work that needs to be done with Dapper to ensure proper use of resources. That might look like this:

using Dapper;

public static class DatabaseAccess
{
    private static string _connString = ConfigurationManager.ConnectionStrings["Default"];

    public static IEnumerable<T> Query<T>(string cmd,
        string filter = null,
        object parameters = null)
    {
        using (SqlConnection c = new SqlConnection(_connString))
        {
            return c.Query<T>(string.Format("{0} {1}", cmd, filter),
                parameters);
        }
    }

    public static T Single<T>(string cmd,
        string filter = null,
        object parameters = null)
    {
        return Query<T>(cmd, filter, parameters).Single();
    }
}

Alright, now that we've got a way to get to the database, let's fix up that POCO a little:

public class Person
{
    private const string _selectCmd = "SELECT personID, name, address FROM Person";

    public int personID {get;set;}
    public string name {get;set;}
    public string address {get;set;}

    public static List<Person> GetPeople()
    {
        return DatabaseAccess.Query<Person>(_selectCmd).ToList();
    }

    public static Person GetPerson(int id)
    {
        return DatabaseAccess.Single<Person>(_selectCmd,
            "WHERE personID = @ID",
            new { ID = id });
    }
}

and so now to use this, you just do this still:

var list = Person.GetPeople();

or to get one:

var p = Person.GetPerson(1);

Now, I could take this even further by using attributes, reflection, and dynamic SQL generation; but I just feel that's probably outside the scope of this post.

\$\endgroup\$
  • \$\begingroup\$ ...and put all the data access code in Person as well? Along with an eventual SaveChanges() or Delete() method? I don't think that's right... \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 13:32
  • \$\begingroup\$ @retailcoder, absolutely. Who else knows how to SELECT, UPDATE, DELETE, or INSERT a Person? Now, I could recommend a more dynamic solution that leverages attributing, reflection, dynamic SQL creation, and Dapper; but that's outside the scope of this question. \$\endgroup\$ – Mike Perrenoud Nov 5 '13 at 13:36
  • 1
    \$\begingroup\$ @retailcoder, first I'm not big on EF. It's bloated and dictates too much. Second, I'm not big on context objects either because they are generally misused (e.g. don't leverage using statements) and are heavy. I prefer Dapper over all solutions, and even in this solution, I'd use Dapper internally to actually SELECT the data. Now, odds are I'd end up building an extension method on IDbConnection to house the actual ADO work and so the method would just about be a one-liner. But the OP is just getting started here; and complicating that with a bloated OO design won't help. \$\endgroup\$ – Mike Perrenoud Nov 5 '13 at 14:01
  • 1
    \$\begingroup\$ @retailcoder, have a look at my edit. I'm going to keep working on it a bit - but I think you'll get the idea. \$\endgroup\$ – Mike Perrenoud Nov 5 '13 at 14:18
  • 1
    \$\begingroup\$ +1 for the effort :) ...and I'm going to look into this Dapper.NET thing, too! \$\endgroup\$ – Mathieu Guindon Nov 5 '13 at 16:46

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.