Should a method that loads a class instance from the database be static or non-static?

Question

Below are two examples of loading a person from the database. I find the static method cleaner to write, but it creates difficulty when using abstract/virtual methods and interfaces.

So which is better practice?

Static:

class Person
{
    public static Person Load(int id)
    {
        // Returns person from database
    }
}

var id = 1;
var person = Person.Load(id);

Non-static:

class Person
{
    public Person Load(int id)
    {
        // Returns person from database
    }
}

var id = 1;
var person = new Person().Load(id);

It makes no sense to have to create a `Person` to load a `Person`, so it should be static. Better still, it should be in a `PersonFactory` class and not a member of `Person` at all. If a `PersonFactory` is something that you might want to pass around (e.g. for Dependency Injection) then `PersonFactory` should NOT be static class, and `PersonFactory.Load()` should NOT be a static method. And then probably go one step further and pass an `IPersonFactory` interface rather than a concrete class. — Matthew Watson, Apr 01 '16 at 14:31
Code review might be a better spot for this http://codereview.stackexchange.com/ — Nikki9696, Apr 01 '16 at 14:31
Neither - have an IPersonFactory, or an IPersonDAO interface, whose job it is to load people. Then have a database-backed implementation of that. Your code will be hugely easier to unit-test. — RB., Apr 01 '16 at 14:31
I'm voting to close this question as off-topic because it belongs to codereview.stackexchange.com — TomTom, Apr 01 '16 at 14:41
@TomTom Where there is more than likely a closely related dupe — TheLethalCoder, Apr 01 '16 at 14:44

Joe Enos · Answer 1 · 2016-04-01T14:40:39.780

I'd keep your data objects dumb and separate, and put your database logic elsewhere, like a repository/factory/manager or whatever you want to call it - and that repository could be where you have your interface and/or inheritance:

public class PersonRepo : BaseRepo, IPersonRepo {
    Person IPersonRepo.Get(int id) {
        // Get from DB
    }
}

Or even make it more generic if you don't have use for custom repositories:

public interface IRepository<T>
{
    T Get(int id);
}

public abstract class BaseRepo<T> : IRepository<T>
{
    public abstract T Get(int id);
}

public class PersonRepo : BaseRepo<Person>
{
    public override Person Get(int id)
    {
        // Get from DB
    }
}

score 0 · Answer 2 · edited May 23 '17 at 11:50

0

Avoid using statics since the code will became not unit testable and tightly coupled.

Use better interfaces and abstract classes to separate behavior and base implementation. Then you will be able to mock you parts with ease.

Also use dependency injection to make code less coupled.

Keep Data Transfer Objects aka Entities (Person class) as simple POCO. And have services (repositories, services, managers) for handling operations with your entities.

Read about SOLID, KISS, DRY, YAGNI and other OOP techniques.

See when to use statics

edited May 23 '17 at 11:50

Community

1
1

answered Apr 01 '16 at 14:34

Sergiy Kozachenko

1,399
11
31

Not sure you are aware of it - but every mocking framework worth a dime can mock static methods. Which means they are unit testable. -1 – TomTom Apr 01 '16 at 14:42
@TomTom you are right, but does it worst to know, that you can write a bad code and then pay for commercial tool that will allow you to test your code, when you can write a good code and tests for free. – Sergiy Kozachenko Apr 01 '16 at 14:48
@TomTom while that's true, it also makes it impossible to generate a dependency graph and utilize good DI patterns. – David L Apr 01 '16 at 14:49

Should a method that loads a class instance from the database be static or non-static?

2 Answers2