How do I design a Data Access Layer appropriately?

Question

I have the following Data Access Layer (DAL). I was wondering if it's set up correctly, or if I need to improve it?

public class User 
{

}

//Persistence methods
static class UserDataAccess
{
   UsersDAL udal = // Choose SQL or FileSystem DAL impl.


   InsertUser(User u)
   {
      // Custom logic , is 'u' valid etc. 

      udal.Insert(u);
   }
}

abstract class UsersDAL
{    
   GetUserByID();
   InsertUser(u);
   ...
}

// implementaitons of DAL

static class UsersSQLStore : UsersDAL
{

}

static class UsersFileSystemStore : UsersDAL
{

}

I separated the storage layer from the User class to access methods collection which further call any custom DAL.

Is use of static in DAL implementation correct?

Please suggest corrections or ways I can make this better. I don't have a lot of experience with writing code in layers.

If you can't take the time to fully spell out your question (using Pl. instead of Please), then how do you expect someone to take the time to answer your question or help you out? — George Stocker, Jan 06 '11 at 18:22
@George, I dont know if that hurts someone, but just to save people reading too much, i use that regularly. Instead i concentrated on writing down my example. That does not mean i dont appreaciate people's time and their responses. — Munish Goyal, Jan 06 '11 at 18:39
Why would you want to do this instead of using an ORM like LLBLGen or Dapper? No need to reinvent the wheel. — Adam Spicer, Jan 24 '12 at 18:57

score 17 · Accepted Answer · answered Jan 06 '11 at 18:25

17

None of those classes should be static. I don't think you should name your classes DAL either, because its short for Data Access Layer, and a class in itself is not a layer (in my mind at least). You can use the widely adopted term repository instead. I suggest you do something like the following:

public class User{

}

public abstract class UserRepository{
    public abstract void InsertUser(User user);
}

public class SqlUserRepository : UserRepository{
    public override void InsertUser(User user)
    {
      //Do it
    }
}

public class FileSystemUserRepository : UserRepository{
    public override void InsertUser(User user)
    {
      //Do it
    }
}

public class UserService{
    private readonly UserRepository userRepository;

    public UserService(UserRepository userRepository){
        this.userRepository = userRepository;
    }

    public void InsertUser(User user){
        if(user == null) throw new ArgumentNullException("user");
        //other checks
        this.userRepository.InsertUser(user);
    }
}

Note that the UserService is injected with an instance of the abstract class UserRepository in its constructor. You can use a Dependency Injection (DI) framework to do this for you automatically, such as Windsor Castle from Castle Project. It will allow you to specify a mapping from abstraction (UserRepository) to concrete implementation (eg. SqlUserRepository) in a configuration file, or in code.

Hope this points you in the right direction, and please ask if you need more information.

answered Jan 06 '11 at 18:25

Klaus Byskov Pedersen

117,245
29
183
222

Thats template pattern. You are in right direction with few changes as suggested by Hoffmann and Jani – hungryMind Jan 06 '11 at 18:30
Great explanation. Why shoudlnt i use static ? If not for repositories, For UserService I can make it static i guess ? 1 more thing (may be unrelated to DAL): If existing user is tried to insert , then who should make this check , UserService or User class itself via constructors (since it doesnt know abt xistence when it is newed) – Munish Goyal Jan 06 '11 at 18:45
@Munish Goyal, you should not use `static` because there is no need for it. When used inappropriately, `static` introduces unnecessary state in your application, which makes it both harder to debug and most importantly, it makes it **very** hard to test automatically. You probably want to read up on the exact meaning of `static` here: http://msdn.microsoft.com/en-us/library/98f28cdx%28vs.71%29.aspx – Klaus Byskov Pedersen Jan 06 '11 at 18:51
1

@Munish Goyal, I think the `UserService` should check if the user already exists, by asking the repository if a user with that id/name already exists. You would need a method called `bool UserAlreadyExists(User user)` in your repositories for this purpose. – Klaus Byskov Pedersen Jan 06 '11 at 18:53
isnt static for common methods where state is not needed. since repository is decided at deploy time once, it wouldnt change and hence there is no state in UserService. static would save object creation/destruction . Pl. correct if wrong. – Munish Goyal Jan 06 '11 at 18:58
@Munish Goyal, basically no. Static classes are also constructed (automatically) and they hold global state. For methods, it's generally ok, but static methods belong to the type and not to an instance, which is definitely not something you would want in this scenario. – Klaus Byskov Pedersen Jan 06 '11 at 19:12
I would use an interface IUserRepository, and a class UserRepository that implements it. If necesary, I'd create an abstract class in the middle, but the abstraction should always be IUserRepository. – bloparod Jan 07 '11 at 03:47
So now the Repository classes (DAL) and Service class has to be in same assembly , right ? Otherwise there wud be circular depend. Is it right to make the service class singleton, because it is a common class to access 'user repository' – Munish Goyal Jan 07 '11 at 08:53
1

@Munish Goyal, no, that is not correct. The abstract UserRepository, the User class, and the UserService must be in the same assembly (AssemblyA). The different implementations of the repositories can be in assemblies of their own (AssemblyB/C). The program that uses the UserService will need a reference to both AssemblyA and AssemblyB (or AssemblyC). – Klaus Byskov Pedersen Jan 07 '11 at 09:23
How to make UserService and hence SqlUserRepository singletons in this case ? (UserService constructor needs repository object as argument) – Munish Goyal Jan 07 '11 at 13:41
@Munish Goyal, don't. Singleton is widely regarded as an **anti-pattern**. – Klaus Byskov Pedersen Jan 07 '11 at 13:46

Jahan Zinedine · Answer 2 · 2012-01-24T18:49:06.923

7

My Humble Opinions

Use interfaces instead of abstract class if User hasn't any hierarchy.
Write a generic DAL so that you can reuse it as a facade for DAL layer.
Resolve the concrete DALs by a DI framework

edited Jan 24 '12 at 18:49

answered Jan 06 '11 at 18:25

Jahan Zinedine

14,616
5
46
70

score 6 · Answer 3 · edited Jun 01 '18 at 12:01

6

Davy Brion has an excellent set of blog posts on this subject: located on GitHub

edited Jun 01 '18 at 12:01

Julian

33,915
22
119
174

answered Jan 06 '11 at 23:25

tijmenvdk

1,773
10
19

3

The link you included is now broken. Please update it. – John Washam Feb 25 '17 at 21:01

How do I design a Data Access Layer appropriately?

3 Answers3