3
\$\begingroup\$

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);
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to code review. The title should reflect what the code is used for, rather than what you are trying to achieve. Know what the code does helps us give a better review. \$\endgroup\$
    – pacmaninbw
    Nov 15 '19 at 15:59
4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ That makes sense, thank you. I've changed the repository registration in StartUp and changed the references to use the injected instance instead, so other than that, looks good? \$\endgroup\$ Nov 15 '19 at 15:46
  • \$\begingroup\$ Yes, it looks good. Although, this is a simple enough app there aren't too many places to make mistakes. \$\endgroup\$
    – user34073
    Nov 15 '19 at 15:47
  • \$\begingroup\$ Yeah fair enough, I'm going to be doing the rest of the application too so it's good to know I'm on the right track at least. Thanks for the input. \$\endgroup\$ Nov 15 '19 at 15:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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