current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
9

I am just trying to use ASP.NET MVC using repository pattern. Am I doing something wrong here?

Model - Contains the model

public class Contact
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string EmailAddress { get; set; }
}

Repository interface - Interface

public interface IContactRepository
{
    void Create(Contact contact);
    void Delete(int contactId);
    void Save(Contact contact);
    Contact Retrieve(int contactId);
    IEnumerable<Contact> Select();
}

Repository class

public class ContactRepository : IContactRepository
{
    private AddressBookDb addressBookdb = new AddressBookDb();

    public void Create(Contact contact)
    {
        addressBookdb.Contacts.Add(contact);
        addressBookdb.SaveChanges();
    }

    public void Save(Contact contact)
    {
        Delete(contact.Id);
        Create(contact);
    }

    public void Delete(int contactId)
    {
        Contact contact = Retrieve(contactId);
        if (contact != null)
        {
            addressBookdb.Contacts.Remove(contact);
            addressBookdb.SaveChanges();
        }
    }

    public Contact Retrieve(int contactId)
    {
        return addressBookdb.Contacts.FirstOrDefault<Contact>(c => c.Id == contactId);
    }

    public IEnumerable<Models.Contact> Select()
    {
        return addressBookdb.Contacts.ToList<Contact>();
    }
}

Controller class

public class ContactController : Controller
{
    IContactRepository contactRepository = null;
    public ContactController() : this (new ContactRepository())
    {

    }

    public ContactController(IContactRepository contactRepository)
    {
        this.contactRepository = contactRepository;
    }

    [HttpPost]
    public ActionResult Edit(Contact contact)
    {
        if (ModelState.IsValid)
        {
            contactRepository.Save(contact);
            return RedirectToAction("Index");
        }
        return View(contact);
    }
}
share|improve this question
    
I dont understand your save method. Why do you delete it before saving??? If youre saving its not even supposed for you to have any ID! And if saving means updating why do you exclude it from db then reinsert? Isnt it messing up with any relationship that might already exists??? –  renatoargh Nov 25 '11 at 1:08
1  
What about moving IContactRepository to a IRepository<T> interface. That way you will not have to implement an interface for every model type you want a repository for? –  dreza Nov 25 '11 at 6:43
    
I believe that the Get() is more conventional than Retrieve() –  Mattias Dec 5 '11 at 13:27

3 Answers 3

up vote 3 down vote

I have a very good solution for you. See the below two blog posts:

Generic Repository Pattern - Entity Framework, ASP.NET MVC and Unit Testing Triangle

How to Work With Generic Repositories on ASP.NET MVC and Unit Testing Them By Mocking

What I can suggest more is this:

Implementing IDisposable:

Implement IDisposable on your repository and dispose them on your controller by overriding the Controller.Dispose method. Sample:

IContactRepository:

public interface IContactRepository : IDisposable {

    //same as above
}

Add the below code to your ContactRepository class:

private bool disposed = false;

protected virtual void Dispose(bool disposing) {

    if (!this.disposed)
        if (disposing)
            addressBookdb.Dispose();

    this.disposed = true;
}

public void Dispose() {

    Dispose(true);
    GC.SuppressFinalize(this);
}

On your controller, add this code:

protected override void Dispose(bool disposing) {

    contactRepository.Dispose();

    base.Dispose(disposing);
}
share|improve this answer
up vote 1 down vote

Couple of things I'd change:

  1. Make save work out whether it needs to create or modify itself.

  2. Don't delete and re-create in the same. It will mess with relational data as Renato said.

  3. In the Retrieve method return SingleOrDefault - Only one contact record should have a unique id. Then let the controller check for null instead of a try/catch which you'd need around a Single call as this would throw an exception if no record was returned. FirstOrDefault makes it look like you're not confident there will be only one record; also depending on ordering it could return a different record depending on conditions.

  4. You could make you save method return a bool value to stop the db side of things throwing an error and crashing the site. You could then do the following:

    [HttpPost]
public ActionResult Edit(Contact contact)
{
    if ((ModelState.IsValid) && (contactRepository.Save(contact)))
    {
        return RedirectToAction("Index");
    }
    return View(contact);
}
share|improve this answer
up vote 0 down vote 
ServiceRequestLocRepository _servicereqlocrepository;

    public DisplayService()
    {
        _context = new VoltEntities();
        _unitOfWork = _context as IUnitOfWork;

        _servicereqlocrepository = new ServiceRequestLocRepository(_unitOfWork);
    }

For call at controller.

share|improve this answer
    
Hi, and welcome to Code Review. I am struggling to understand how this relates to the original question, and have given up. Please add more context to your answer so it is clearer how this is a review of the original question. –  rolfl♦ Jun 7 at 14:22

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.