I am working on a asp.net mvc project. And I wonder if the setup of the project is correct. So just some advice

Because I see this:

public IndicatorController(
    IndicatorService indicatorService,
    PatientLogService patientLogService,
    PatientDbContext dbContext,
    AppIdentityDbContext identityDbContext,
    MedicalDbContext medicalDbContext,
    IMeasurementProcessor measurementProcessor,
    IMeasurementRepository measurementRepository,
    IEntryRepository entryRepository,
    IPersonalFieldBoundaryRepository personalFieldBoundaryRepository,
    IEmailSender emailSender,
    IndicatorActionService indicatorActionService,
    IndicatorBuilderContext indicatorBuilderContext,
    IPatientIndicatorService patientIndicatorService,
    ViewRender viewRender,
    IEmailSender messageSender,
    IOptions<AppSettings> appSettings,
    ILoggerFactory loggerFactory)
    : base(dbContext, identityDbContext, medicalDbContext, loggerFactory)
    _patientLogService = patientLogService;
    _indicatorService = indicatorService;
    _measurementProcessor = measurementProcessor;
    _measurementRepository = measurementRepository;
    _entryRepository = entryRepository;
    _personalFieldBoundaryRepository = personalFieldBoundaryRepository;
    _emailSender = emailSender;
    _indicatorActionService = indicatorActionService;
    _indicatorBuilderContext = indicatorBuilderContext;
    _patientIndicatorService = patientIndicatorService;
    _viewRender = viewRender;
    _messageSender = messageSender;
    _appSettings = appSettings.Value;

And the base Controller looks like this:

public class BaseController : Controller
    protected PatientDbContext _patientDbContext;
    protected MedicalDbContext _medicalDbContext;
    protected readonly AppIdentityDbContext _identityDbContext;

    protected readonly ILogger _logger;

    public string CurrentUserId => User.FindFirstValue(JwtClaimTypes.Subject);

    private Task<Participant> _participant;
    private Task<UserIdentity> _userIdentity;

    public Task<Participant> Participant => _participant ?? (_participant = LoadParticipant());

    public Task<UserIdentity> UserIdentity => _userIdentity ?? (_userIdentity = LoadUserIdentity());

    public BaseController(
        PatientDbContext patientDbContext,
        AppIdentityDbContext identityDbContext,
        MedicalDbContext medicalDbContext,
        ILoggerFactory loggerFactory)
        _patientDbContext = patientDbContext;
        _medicalDbContext = medicalDbContext;
        _identityDbContext = identityDbContext;
        _logger = loggerFactory.CreateLogger<BaseController>();

    private async Task<Participant> LoadParticipant()
        return !RouteData.Values.ContainsKey("participantId")
            || !Guid.TryParse(RouteData.Values["participantId"].ToString(), out Guid participantId)
            ? null : await _patientDbContext.Participants.FindAsync(participantId);

    private async Task<UserIdentity> LoadUserIdentity()
        var participant = await Participant;
        return await _identityDbContext.Users
            .Include(i => i.ParticipantIdentity)
            .FirstOrDefaultAsync(i => i.Id == participant.UserIdentityId);

But so my question is: Is this the correct way to inject the PatientDbContext dbContext directly into the controllers constructor?

Thank you

But for example the AdviseService , there you directly inject the dbcontext:

public class AdviceService
    private static readonly HtmlSanitizer s_htmlSanitizer = HtmlSanitizerProvider.GetSanitizer();

    private readonly PatientDbContext _patientDbContext;
    private readonly MedicalDbContext _medicalDbContext;
    private readonly AppIdentityDbContext _identityDbContext;
    private readonly AdviceLayoutFactory _adviceLayoutFactory;
    private readonly PatientLogService _patientLogService;
    private readonly ILogger _logger;
    private readonly JsonSerializerSettings _jsonSerializerSettings;
    private readonly ChatDbContext _chatDbContext;

    public AdviceService(
        PatientDbContext patientDbContext,
        MedicalDbContext medicalDbContext,
        ChatDbContext chatDbContext,
        AppIdentityDbContext identityDbContext,
        AdviceLayoutFactory adviceLayoutFactory,
        PatientLogService patientLogService,
        ILoggerFactory loggerFactory,
        JsonSerializerSettings jsonSerializerSettings)
        _patientDbContext = patientDbContext;
        _medicalDbContext = medicalDbContext;
        _identityDbContext = identityDbContext;
        _adviceLayoutFactory = adviceLayoutFactory;
        _patientLogService = patientLogService;
        _jsonSerializerSettings = jsonSerializerSettings;
        _logger = loggerFactory.CreateLogger<AdviceService>();
        _chatDbContext = chatDbContext;
  • 7
    \$\begingroup\$ Could you explain a bit what this project is about? \$\endgroup\$ – dfhwze Jul 24 '19 at 14:58
  • \$\begingroup\$ it is the backend of a Angular project. But What exactly do you mean? \$\endgroup\$ – mightycode Newton Jul 24 '19 at 15:22
  • 7
    \$\begingroup\$ Just a small description of the app you are building would do. But what we are really missing, is how you actually inject these dependencies. Do you use a Depencency Injection framework ? \$\endgroup\$ – dfhwze Jul 24 '19 at 15:24

Welcome to CodeReview :-)

Code to contract (interface) not to implementation

Your program components should be loosely coupled. It is important for two main reasons:

  • you can easily swap components of your program.
  • change in one place would not force you to alter code independent components

What does this mean in practice?

Start with defining interfaces. For starter, they can be 1:1 copy of your public methods signatures. Then register them in your DI container and pass interfaces into the constructor. (ie. IAdviceLayoutFactory, IPatientLogService instead of AdviceLayoutFactory, PatientLogService)

Single Responsibility Principle

In simple terms, classes should be small and have one purpose. When someone asks you - what does class X do? you should be able to answer with a simple sentence, containing one verb.

Another rule of thumb - if a class has more than 6 parameters in a constructor it should be split.

A good example of counter-example would be IndicatorController. It has like 20 parameters. I presume it can't be described with a simple sentence.

Controllers have only one purpose - call application (domain) logic with parameters deserialized from the network request. As simple as it sounds:

public IndicatorController(IIndicatorService service) {
    _service = service;

// Methods matching endpoints related to Indicators

Code has to be testable

When you follow the rules mentioned above your code is match more testable.

With interfaces, you can easily replace actual implementation with a fake one. Eg. Instead of connecting to a real database you can simply create a fake implementation of class responsible for data manipulation. With dependence on interfaces and some mocking library (like Moq) it is super easy.

Thanks to smaller classes it would be easier to focus on a single feature of your app and unit test it with less moving parts. Just moq all code on which your feature depends.

I would like to give you a change to refactor your code a bit. If you are stuck I would be glad to help - I just don't want to steal all the fun :-)

| improve this answer | |
  • \$\begingroup\$ Thank you for your time and effort too show it how it can be better structured. \$\endgroup\$ – mightycode Newton Jul 25 '19 at 7:22
  • \$\begingroup\$ You are welcome. Feel free to ask if you have any questions. \$\endgroup\$ – Piotr Nawrot Jul 25 '19 at 23:14

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