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.

Solution 1

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.

Solution 2

My Humble Opinions

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

Solution 3

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

Comments

[George Stocker]

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?

[Munish Goyal]

@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.

[hungryMind]

Thats template pattern. You are in right direction with few changes as suggested by Hoffmann and Jani

[Munish Goyal]

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)

[Klaus Byskov Pedersen]

@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: msdn.microsoft.com/en-us/library/98f28cdx%28vs.71%29.aspx

[Klaus Byskov Pedersen]

@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.

[Munish Goyal]

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.

[Klaus Byskov Pedersen]

@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.

[bloparod]

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.

[Munish Goyal]

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'

[Klaus Byskov Pedersen]

@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).

[Munish Goyal]

How to make UserService and hence SqlUserRepository singletons in this case ? (UserService constructor needs repository object as argument)

[Klaus Byskov Pedersen]

@Munish Goyal, don't. Singleton is widely regarded as an anti-pattern.

[John Washam]

The link you included is now broken. Please update it.