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 GetAllReservations()
        {

            List 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 GetAllReservations();
}

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

public class MyOracleReservationRepository : IReservationRepository
{
public List GetAllReservations()
{

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 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 GetAllReservations()
        {
            List 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 GetReservationsFromDatabase()
        {
            int databaseValueResID1 = 1;
            DateTime databaseValueResDate1 = System.Convert.ToDateTime("1/1/2001");
            int databaseValueResBookID1 = 101;

            List reservations = new List();

            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 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 sho

StackExchange Code Review Q#12730, answer score: 2