Good repository pattern for asp.net mvc

Question

I have implemented a repository pattern in my asp.net mvc web application... But i want to know is this a good repository pattern or still can i improve it more...

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using TaxiMVC.BusinessObjects;

namespace TaxiMVC.Models
{
    public class ClientRepository
    {
        private TaxiDataContext taxidb = new TaxiDataContext();
        Client cli = new Client();

        //Get all Clients
        public IQueryable<ClientBO> FindAllClients(int userId)
        {
            var client = from c in taxidb.Clients
                         where c.CreatedBy == userId && c.IsDeleted == 0 
                         select new ClientBO()
                         {
                             ClientId = c.ClientId,
                             ClientName= c.ClientName,
                             ClientMobNo= Convert.ToString(c.ClientMobNo),
                             ClientAddress= c.ClientAddress
                         };
            return client;
        }

        //Get Client By Id
        public ClientBO FindClientById(int userId,int clientId)
        {
            return (from c in taxidb.Clients
                    where c.CreatedBy == userId && c.ClientId == clientId && c.IsDeleted == 0
                         select new ClientBO()
                         {
                             ClientId = c.ClientId,
                             ClientName= c.ClientName,
                             ClientMobNo= Convert.ToString(c.ClientMobNo),
                             ClientAddress= c.ClientAddress
                         }).FirstOrDefault();
        }

        //Insert a new client
        public bool ClientInsert(ClientBO clientBO)
        {
            cli.ClientName = clientBO.ClientName;
            cli.ClientMobNo = Convert.ToInt64(clientBO.ClientMobNo);
            cli.ClientAddress = clientBO.ClientAddress;
            cli.CreatedDate = clientBO.CreatedDate;
            cli.IsDeleted = clientBO.IsDeleted;
            cli.CreatedBy = clientBO.CreatedBy;

            if (!taxidb.Clients.Where(c => c.ClientMobNo == cli.ClientMobNo).Any())
            {
                taxidb.Clients.InsertOnSubmit(cli);
                taxidb.SubmitChanges();
                return true;
            }
            else
                return false;
        }

      //Client Update
        public ClientBO updateClient(ClientBO clientBO)
        {
            var table = taxidb.GetTable<Client>();
            var cli = table.SingleOrDefault(c => c.ClientId == clientBO.ClientId && c.CreatedBy==clientBO.CreatedBy);
            cli.ClientName = clientBO.ClientName;
            cli.ClientMobNo = Convert.ToInt64(clientBO.ClientMobNo);
            cli.ClientAddress = clientBO.ClientAddress;
            taxidb.SubmitChanges();
            return clientBO;
        }

        //Delete Clients
        public bool deleteClients(string Ids, int userId)
        {
            var idsToDelete = Ids.Split(',').Select(c => Convert.ToInt32(c));
            var clientsToDelete = taxidb.Clients.Where(c => idsToDelete.Contains(c.ClientId));
            foreach (var client in clientsToDelete)
            {
                client.IsDeleted = Convert.ToByte(1); 
            }
            taxidb.SubmitChanges();
            return true;
        }
     }
}

and my ClientBo.cs,

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;

namespace TaxiMVC.BusinessObjects
{
    public class ClientBO
    {
        public int ClientId { get; set; }
        public string ClientName { get; set; }
        public string ClientMobNo { get; set; }
        public string ClientAddress { get; set; }
        public DateTime CreatedDate { get; set; }
        public byte IsDeleted { get; set; }
        public int CreatedBy { get; set; }
    }
}

I Didn't implement an IRepository here... Should i ve to implement it or should my repository can still be improved... Any suggestion....

Answer 1

Hmm, there is definitely a couple of things that I would do to improve this.

First: I would define an interface for your Repository to implement. This allows you greater control over dependencies, and when coupled with a Inversion Of Control/Dependency Injection (IOC/DI) framework, this is hugely improved. IOC/DI frameworks include StructureMap or NInjet. Have a read of this from Scott Hanselman, it's a pretty comprehensive list.

Your interface may look like:

public interface IClientRepository
{
    IQueryable<ClientBO> FindAllClients(int userId);
    ClientBO FindClientById(int userId, int clientId);
    ClientInsert(ClientBO clientBO);
    ClientBO updateClient(ClientBO clientBO);
    bool deleteClients(string Ids, int userId);
}

Second: don't do your Business Object (ClientBO) to persistent object (Client) conversion inside of your repository. This means that if you make any changes to your BO, then you'll need to go through and change your entire repository.

I notice you have a lot of left-right assignment code, eg.

cli.ClientName = clientBO.ClientName;

I would seriously investigate the use of AutoMapper. It makes this "monkey code" a hell of a lot easier.

EDIT: Here is a blog post that describes how to use AutoMapper to remove the left-right assignment code.

Third: Your naming structure is all over the shop. We have: FindAllClients(), ClientInsert(), updateClient() all in the one class. Very very poor naming. For your repository, try to model your methods on what will be happening on the DB side. Try Add or Insert, Delete, FindAll or GetAll, Find or GetAll, SaveChanges, method names.

Don't append/prepend the type to the method name, as your are in the ClientRepository, it's implied that you'll be adding or getting Client's.

Fourth: Your mixing your LINQ syntax. In some places your using the declarative query syntax and other places your using the method syntax. Pick 1 and use it everywhere.

Fifth: This line worries me:

 if (!taxidb.Clients.Where(c => c.ClientMobNo == cli.ClientMobNo).Any())

This looks suspiciously like business logic to me. Not something that should be in the repository. Either declare the column to be UNIQUE in the DB, or move that logic into another validation layer.

These were the main things that jumped out at me. A couple of these are personal preference, but I hope this helps.

Answer 2

Why did you use string for array of Id:

public bool deleteClients(string Ids, int userId)

I think you should use int[] or List<int>.

Also I'd write List but not IQueryable in FindAllClients.

public List<ClientBO> FindAllClients(int userId)

And of course you should write interface IClientRepository because it's right approach according to SOLID principles.

If you'll write interface, then you can write other classes which use Repository with this interface more flexibly, something like this:

public class ClientService : IClientService
{
    IClientRepository m_clientRepository;

    public ClientService(IClientRepository rep)
    {
        m_clientRepository = rep;
    }
}

And then you will be able to test this code with Mock-class IClientRepository using something like Moq. And of course, if you'll write other IClientRepository(for example, XmlClientRepository) you'll only change initialization of class which use repository.

If you use ASP.NET MVC you'll be able to use IClientRepository in your controller, and it will be more testable.

For more flixible solution you could use IoC-container pattern(NInject, Unity).

I hope this answer will help you.