2
\$\begingroup\$

Background

An API endpoint that returns phone numbers. I would appreciate some feedback on this very simple service class that is part of the overall solution. Here is its Interface:

public interface IPhoneNumbersService
{
    Task<IList<PhoneNumberViewModel>> GetAllPhoneNumbersAsync();

    Task<IList<CustomerPhoneNumberViewModel>> GetCustomerPhoneNumbersAsync(int customerId);
}

And here is its implementation:

public class PhoneNumbersService : IPhoneNumbersService
{
    private ZetaContext context;

    private readonly ILogger logger;

    public PhoneNumbersService(
        ZetaContext context,
        ILogger<PhoneNumbersService> logger)
    {
        this.context = context;
        this.logger = logger;
    }

    public async Task<IList<PhoneNumberViewModel>> GetAllPhoneNumbersAsync()
    {
        List<PhoneNumberViewModel> phoneNumbers = await context
            .PhoneNumbers
            .AsNoTracking()
            .Select(x => new PhoneNumberViewModel
            {
                Customer = $"{x.Customer.Title} {x.Customer.FirstName} {x.Customer.LastName}",
                PhoneNumber = x.Value
            })
            .ToListAsync();

        logger.LogTrace($"{phoneNumbers.Count} phone numbers were retrieved from the database.");

        return phoneNumbers;
    }

    public async Task<IList<CustomerPhoneNumberViewModel>> GetCustomerPhoneNumbersAsync(int customerId)
    {
        Customer customer = await context
            .Customers
            .AsNoTracking()
            .Include(x => x.PhoneNumbers)
            .Where(x => x.Id == customerId)
            .SingleOrDefaultAsync();

        if (customer == null)
        {
            throw new CustomerNotFoundException(customerId);
        }

        List<CustomerPhoneNumberViewModel> customerPhoneNumbers = customer
            .PhoneNumbers
            .Select(x => new CustomerPhoneNumberViewModel
            {
                PhoneNumber = x.Value,
                IsActive = x.IsActive,
                CreatedAt = x.CreatedAt
            })
            .ToList();

        return customerPhoneNumbers;
    }
}

The Controller that calls the above service looks like this:

[ApiController]
public class PhoneNumbersController : ControllerBase
{
    private readonly IPhoneNumbersService phoneNumbersService;

    private readonly ILogger logger;

    public PhoneNumbersController(
        IPhoneNumbersService phoneNumbersService,
        ILogger<PhoneNumbersController> logger)
    {
        this.phoneNumbersService = phoneNumbersService;
        this.logger = logger;
    }

    [Route("api/numbers/")]
    public async Task<IActionResult> GetAllPhoneNumbersAsync()
    {
        IList<PhoneNumberViewModel> phoneNumbers = await phoneNumbersService.GetAllPhoneNumbersAsync();

        return Ok(phoneNumbers);
    }

    [Route("api/customers/{customerId}/numbers")]
    public async Task<IActionResult> GetCustomerPhoneNumbers(int? customerId)
    {
        if (!customerId.HasValue || customerId.Value == 0)
        {
            throw new ArgumentNullException(nameof(customerId));
        }

        try
        {
            IList<CustomerPhoneNumberViewModel> customerPhoneNumbers = await phoneNumbersService.GetCustomerPhoneNumbersAsync(customerId.Value);

            return Ok(customerPhoneNumbers);

        }
        catch (CustomerNotFoundException exception)
        {
            logger.LogError($"The customer with Id: {exception.CustomerId} could not be found.");

            return NotFound();
        }
    }
}

One thing I am not sure about: Where should one convert the Entity PhoneNumber.cs to its ViewModel PhoneNumberViewModel.cs. Should this happen in the Service class like it is now (see .Select())? Or should this happen in the controller?

If in the controller, then that means my service method GetAllPhoneNumbersAsync() should return a list (or should it be IEnumerable or ICollection) of the entities to the controller. Is that bad? Some say that is leaking my entities into the controller, others say the service should be as re-usable as possible and shouldn't do any mapping.

For an Enterprise application, what more can you do to improve the above? Any tips and pointers would be greatly appreciated.

\$\endgroup\$
2
\$\begingroup\$

Design

The API should preferrably consist of its own contracts (interfaces and DTO's). The problem with your API is that it returns data that is too much flattened to be usable for consumers other than your intended application.

Ask yourself this:

  • What if someone wants a different string layout for PhoneNumberViewModel.Customer?
 .Select(x => new PhoneNumberViewModel
    {
        // A specific string concatenation is bad practice for an API ->
        Customer = $"{x.Customer.Title} {x.Customer.FirstName} {x.Customer.LastName}",
        PhoneNumber = x.Value
    })

 public class PhoneNumberViewModel // <- make a DTO from this
    {
        public string PhoneNumber { get; set; }
        public string Customer { get; set; } // <- reference a Customer DTO here
    }

Tips

  • Make your own DTO objects, keep OO-principles into account (don't flatten objects to string)
  • Use dedicated mappers from/to your domain or DAO and the DTO. You could use a framework for this.

Argument Guards

Distinguish arguments that are not set from those that have an incorrect value. There is also a simpler way of checking a Nullable<> against either null or a value.

if (!customerId.HasValue || customerId.Value == 0)
{
      throw new ArgumentNullException(nameof(customerId));
}
if (customerId == null) // <- calls customerId.HasValue
{
      throw new ArgumentNullException(nameof(customerId));
}
if (customerId == 0) // <- calls customerId.Value
{
      throw new ArgumentException("The value cannot be 0.", nameof(customerId));
}

Q&A

Where should one convert the Entity PhoneNumber.cs to its ViewModel PhoneNumberViewModel.cs?

Convert data to view models on the client. PhoneNumberViewModel should not be a contract in your service at all. Instead, create a DTO and let your controller map from DAO to DTO. In enterprise code, however, you might want to use multiple layers and refactor your server alltogether.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much @dfhwze. I would like something clarified please, by Data Access Object, do you mean my Entity? The thing defined as DbSet<Entity> in the DbContext class? If yes, then isn't that what people mean by "leaking entities" into the controller class? I mean sure, you are then mapping the Entity to a DTO and returning the DTO, but still, isn't that leaking the entities? \$\endgroup\$ – J86 May 31 at 7:30
  • \$\begingroup\$ @J86 The way I would handle it is (1) having a Data Layer that works with Entity/DAO classes (2) having a Domain Layer which works domain entities and maps from/to the Data Layer and (3) having a Service Layer that works with DTO entities and maps from/to the Domain Layer. Exceptions to these tiered layers can be made for CQRS, ODATA, and other querayble patterns. \$\endgroup\$ – dfhwze May 31 at 7:37
  • \$\begingroup\$ So to answer your question, your controller would not know your Entities in a n-tiered application. \$\endgroup\$ – dfhwze May 31 at 7:38
  • \$\begingroup\$ Ok, thank you, so the service would actually return the PhoneNumberDTO. In a simple case such as this, do I need a Domain layer? I mean even if I did, would I gain anything? Because the PhoneNumberEntity.cs (assume I rename my entities to end with the word Entity) would be a direct map to PhoneNumber.cs in my .Domain project. \$\endgroup\$ – J86 May 31 at 7:43
  • \$\begingroup\$ @J86 You proably don't need a Domain Layer for this. Only when you're extending your API with CRUD operations I would consider adding in a Domain Layer. But without Domain Layer, your Service Layer is responsible for mapping entities to DTO's. \$\endgroup\$ – dfhwze May 31 at 7:44

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.