Refactoring code to implement clean code using repositories

Question

I've been refactoring code today, try to implement a clean code setup using repositories. Is this on track? Can any improvements be made?

public class IndexModel : PageModel
{
    private readonly MembershipFormRepository _membershipFormRepository;
    private readonly IMembershipFormViewModelService _membershipFormViewModelService;

    public IndexModel(IMembershipFormViewModelService membershipFormViewModelService, MembershipFormRepository membershipFormRepository)
    {
        _membershipFormViewModelService = membershipFormViewModelService;
        _membershipFormRepository = membershipFormRepository;
    }

    [BindProperty]
    public MembershipFormViewModel MembershipFormModel { get; set; } = new MembershipFormViewModel();

    public async Task OnGetAsync()
    {
        MembershipFormModel = await _membershipFormViewModelService.GetViewModel();
    }

    public async Task<IActionResult> OnPostAddMemberAsync()
    {
        if (!ModelState.IsValid)
        {
            return Page();
        }

        await _membershipFormRepository.AddMember(MembershipFormModel);

        return RedirectToPage("Index");
    }
}

MembershipFormViewModelService

public class MembershipFormViewModelService : IMembershipFormViewModelService
{
    private readonly MembershipContext _context;
    private readonly MembershipFormRepository _membershipFormRepository;


    public MembershipFormViewModelService(MembershipContext context, MembershipFormRepository membershipFormRepository)
    {
        _context = context;
        _membershipFormRepository = membershipFormRepository;
    }

    public async Task<MembershipFormViewModel> GetViewModel()
    {
        var viewModel = new MembershipFormViewModel()
            {
                Titles = await SetTitlesDropDown(),
            };

        return viewModel;
    }

    private async Task<IEnumerable<SelectListItem>> SetTitlesDropDown()
    {
        var titlesList = new List<SelectListItem>();

        var titles = await _membershipFormRepository.GetTitles();

        titlesList.AddRange(titles.Select(title => new SelectListItem() {
            Value = title.TitleId.ToString(),
            Text = title.Title
        }));

        return titlesList;
    }
}

IMembershipFormViewModelService

public interface IMembershipFormViewModelService
{
    Task<MembershipFormViewModel> GetViewModel();
}

MembershipFormRepository

public class MembershipFormRepository : IMembershipFormRepository
{
    private readonly MembershipContext _context;
    private readonly IMapper _mapper;

    public MembershipFormRepository(MembershipContext context, IMapper mapper)
    {
        _context = context;
        _mapper = mapper;
    }

    public async Task<IEnumerable<Title>> GetTitles()
    {
        return await _context.Title.ToListAsync();
    }

    public async Task AddMember(MembershipFormViewModel membershipForm)
    {
        var membership = new Membership(); 

        membership = _mapper.Map(membershipForm, membership);

        _context.Membership.Add(membership);

        await _context.SaveChangesAsync();
    }
}

IMembershipFormRepository

public interface IMembershipFormRepository
{
    Task<IEnumerable<Title>> GetTitles();
    Task AddMember(MembershipFormViewModel membershipForm);
}

Answer 1

You are close. The only thing that jumps out at me is your use of the MembershipFormRepository type instead of the IMembershipFormRepository interface. This prevents you from being able to test consumers of it easily. For example, consider this case:

You have an object of type Foo that takes an injected instance of type Bar that takes an injected instance of a DB context context. You want to test that something on the Foo type works. Here is your test setup:

var dbContext = new Moq<DbContext>();
dbContext
    .Setup(s => s.Get())
    .Returns(new List<Item>());

var bar = new Bar(dbContext.Object);
var foo = new Foo(bar);

// assert foo.DoSomething()

Notice how you have to mock up a full Bar object, and even more troublesome, rely on Bar working correctly, for the test to pass. If Bar has a bug, your test can fail; this test, because it tests Foo, should only fail if Foo has a bug. If you injected a type of IBar into the Foo instead, you could mock that, instead of the DB context:

var bar = new Moq<IBar>();
bar.Setup(s => s.GetItems())
   .Returns(new List<Item>());

var foo = new Foo(bar.Object);

// assert foo.DoSomething()

Now, instead of relying on Bar.GetItems not being buggy, you just create a mocked object that always returns the same data. This test also won't fail if Bar changes. The first test would fail if you changed Bar.GetItems to call DbContext.Where instead of DbContext.Get because you hadn't mocked the Where call, which would once again falsely report a bug in the Foo type because the Bar type had changed or was buggy.