API endpoint that returns phone numbers

Question

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.

Answer 1

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.