Repository Pattern Review I have following code for a library management system.


Is the following code a proper implementation of the Repository Pattern?
How can I adapt this code to use generics?
Should I let MyOracleReservationRepository and MyOracleBookRepository be  DataContracts if I am consuming this code as a WCF service (business Layer will be called by service layer)? 


Business Layer

namespace LibraryBL
{
    public class ReservationManager
    {
        //LibraryDAL.ReservationDAL resDAL = new LibraryDAL.ReservationDAL();
        //LibraryDAL.BookDAL bookDAL = new LibraryDAL.BookDAL();

        LibraryRepository.IReservationRepository reservationRepository;
        LibraryRepository.IBookRepository bookRepository;


        public ReservationManager(LibraryRepository.IReservationRepository resRepositroy, LibraryRepository.IBookRepository bookRepositroy)
        {
            reservationRepository = resRepositroy;
            bookRepository = bookRepositroy;
        }

        public List&lt;LibraryDTO.Reservation&gt; GetAllReservations()
        {

            List&lt;LibraryDTO.Reservation&gt; allReservations = reservationRepository.GetAllReservations();

            //Book object inside allReservations has two values as NULL (author and BookTitile).
            //These values need to be set using foreach loop
            foreach (LibraryDTO.Reservation reservation in allReservations)
            {
                int bookID =reservation.ReservedBook.BookID;
                LibraryDTO.Book book = bookRepository.GetBookByID(bookID);
                reservation.ReservedBook = book;
            }
            return allReservations;
        }

    }

}


Repository

namespace LibraryRepository
{
    public interface IReservationRepository
    {
        List&lt;LibraryDTO.Reservation&gt; GetAllReservations();
    }

    public interface IBookRepository
    {
        LibraryDTO.Book GetBookByID(int bookID);
    }


    public class MyOracleReservationRepository : IReservationRepository
    {
        public List&lt;LibraryDTO.Reservation&gt; GetAllReservations()
        {
            int databaseValueResID1 = 1;
            DateTime databaseValueResDate1 = System.Convert.ToDateTime("1/1/2001");
            int databaseValueResBookID1 = 101;

            List&lt;LibraryDTO.Reservation&gt; reservations = new List&lt;LibraryDTO.Reservation&gt;();

            LibraryDTO.Reservation res = new LibraryDTO.Reservation();
            res.ReservationID = databaseValueResID1;
            res.ReservedDate = databaseValueResDate1;
            res.ReservedBook = new LibraryDTO.Book();
            res.ReservedBook.BookID = databaseValueResBookID1;
            res.ReservedBook.Author = null; //Set as null as we don't have values in Reservation DAL
            res.ReservedBook.BookTitle = null; //Set as null as we don't have values in Reservation DAL

            reservations.Add(res);
            return reservations;
        }
    }

    public class MyOracleBookRepository : IBookRepository
    {
        public LibraryDTO.Book GetBookByID(int bookID)
        {
            LibraryDTO.Book book = null;
            if (bookID == 101)
            {
                book = new LibraryDTO.Book();
                book.BookID = 101;
                book.Author = "First Author";
                book.BookTitle = "Book 1";
            }
            return book;
        }
    }
}
That is a very good start.  However, look into moving the loop to get all the books into the GetAllReservations() method in the concrete class that implements IReservationRepository.  When I see the method name "GetAllReservations" in the interface IReservationRepository I would expect it would return all the reservation DTOs fully populated.  You can still keep the logic separate in the repository, it would just have a call to GetByBookId method.  This means the constructor on MyOracleReservationRepository would have to change, but you can hide that in a factory.  

namespace LibraryRepository
{
    public interface IReservationRepository
    {
        List&lt;LibraryDTO.Reservation&gt; GetAllReservations();
    }

    public interface IBookRepository
    {
        LibraryDTO.Book GetBookByID(int bookID);
    }

    public class LibraryRepositoryFactory
    {
        public virtual IReservationRepository GetReservationRepository()
        {
            IBookRepository bookRepo = GetBookRepository();

            return new MyOracleReservationRepository(bookRepo);
        }

        public virtual IBookRepository GetBookRepository()
        {
            return new MyOracleBookRepository();
        }
    }

    public class MyOracleReservationRepository : IReservationRepository
    {
        private IBookRepository m_BookRepo;

        public MyOracleReservationRepository(IBookRepository bookRepo)
        {
            m_BookRepo = bookRepo;
        }

        public List&lt;LibraryDTO.Reservation&gt; GetAllReservations()
        {
            List&lt;LibraryDTO.Reservation&gt; reservations = GetReservationsFromDatabase();

            foreach (LibraryDTO.Reservation reservation in allReservations)
            {
                int bookID =reservation.ReservedBook.BookID;
                LibraryDTO.Book book = bookRepository.GetBookByID(bookID);
                reservation.ReservedBook = book;
            }
            return allReservations;
        }

        private List&lt;LibraryDTO.Reservation&gt; GetReservationsFromDatabase()
        {
            int databaseValueResID1 = 1;
            DateTime databaseValueResDate1 = System.Convert.ToDateTime("1/1/2001");
            int databaseValueResBookID1 = 101;

            List&lt;LibraryDTO.Reservation&gt; reservations = new List&lt;LibraryDTO.Reservation&gt;();

            LibraryDTO.Reservation res = new LibraryDTO.Reservation();
            res.ReservationID = databaseValueResID1;
            res.ReservedDate = databaseValueResDate1;
            res.ReservedBook = new LibraryDTO.Book();
            res.ReservedBook.BookID = databaseValueResBookID1;
            res.ReservedBook.Author = null; //Set as null as we don't have values in Reservation DAL
            res.ReservedBook.BookTitle = null; //Set as null as we don't have values in Reservation DAL

            reservations.Add(res);
            return reservations;
        }
    }

    public class MyOracleBookRepository : IBookRepository
    {
        public LibraryDTO.Book GetBookByID(int bookID)
        {
            LibraryDTO.Book book = null;
            if (bookID == 101)
            {
                book = new LibraryDTO.Book();
                book.BookID = 101;
                book.Author = "First Author";
                book.BookTitle = "Book 1";
            }
            return book;
        }
    }
}


Now your ReservationManager would look something like this:

namespace LibraryBL
{
    public class LibraryBLFactory
    {
        public ReservationManager GetReservationManager()
        {
            var repoFactory = new LibraryRepository.LibraryRepositoryFactory();
            LibraryRepository.IReservationRepository reservationRepo = repoFactory.GetReservationRepository();

            return new ReservationManager(reservationRepo);
        }
    }

    public class ReservationManager
    {
        LibraryRepository.IReservationRepository reservationRepository;


        public ReservationManager(LibraryRepository.IReservationRepository resRepositroy)
        {
            reservationRepository = resRepositroy;               
        }

        public List&lt;LibraryDTO.Reservation&gt; GetAllReservations()
        {
            return reservationRepository.GetAllReservations();
        }

    }

}


I am sure there is a bit of refactoring that can be done but hopefully you get the idea. 

The idea of the repository is to hide how the data is stored and retrieved from other components.  In this particular example ReservationManager.  How would the ReservationManager know that after it got all the reservations it would need to go back to the repository and load all the books?  What if there was another consumer of the repository?  How would it know to do that?  The fact you added a comment to indicate why a for loop is needed should be an indication the loop was not in the right place.

As for your other questions.  I am not sure what your intention is for Generics, I cannot see a good place where it could be used.  As for using data contracts, I would not be in favor of that.  The LibraryBL namespace should exist in it's own set of class libraries another service library should invoke through interfaces.  The service library should be the one consumed directly by the WCF service.  And it would map your DTOs to the Data Contracts.  This will keep your code decoupled, because if you ever had to change your logic in LibraryBL you wouldn't really have to worry about how it affects the WCF service, unless you alter a property in the DTOs, but then you would only have to worry about the mapping.