I am trying to complete a challenge - a simple .net core API with one simple endpoint. This endpoint should validate the user's information and return a unique ID. He should provide: 1) Name, 2) Surname, 3) Reference number, 4) Either the user's Date Of Birth OR the policy holder’s email:
- Insurance holder’s name and surname - required and should be between 3 and 50 chars.
- Insurance number is required and should match Two capitalised alpha character followed by a hyphen and 6 numbers. - the first question: If supplied the user's DOB should mean the customer is at least 18 years old when he is registering.
I would so much appreciate any advice on how I can improve the solution, regarding .net core API best practices, asynchronous programming, design and so on. Also, I am using EF Core for the first time, any suggestions about modification of the dataset or repository changes are welcomed.
What I have done:
- Api controller
[ApiController] [Route("api/[controller]")] public class CustomerController : ControllerBase { private readonly ICustomerService _customerService; private readonly ILogger<CustomerController> _logger; public CustomerController(ICustomerService customerService, ILogger<CustomerController> logger) { _customerService = customerService; _logger = logger; } [HttpPost("create")] public async Task<IActionResult> CreateCustomer([FromBody] CustomerDto customer) { await _customerService.AddCustomerAsync(customer);> return CreatedAtRoute("GetBook", new { id = customer.Id }); } }
As you see it accepts CustomerDto as input. Here is what validation I have supplied:
public class CustomerDto { public int Id { get; set; } [Required] [StringLength(50, MinimumLength = 3, ErrorMessage = "First Name should be between 3 and 50 character in length.")] public string Name { get; set; } [Required] [StringLength(50, MinimumLength = 3, ErrorMessage = "Surname should be > between 3 and 50 character in length.")] public string Surname { get; set; } [Required(ErrorMessage = "Policy Reference Number is required.")] [RegularExpression(@"^[A-Z]{2}-\d{6}$", ErrorMessage = "Policy Reference Number must be in the format XX-999999.")] public string Reference { get; set; } [RequiredIf("Email")] public DateTime? DateOfBirth { get; set; } [RequiredIf("DateOfBirth")]> public string? Email { get; set; } } }
- CustomerService, which only maps the parameter to the Customer DataSet object and calls the repository:
public class CustomerService : ICustomerService { private readonly ICustomerRepository _customerRepository; private readonly IMapper _mapper; public CustomerService(ICustomerRepository customerRepository, IMapper mapper) { _customerRepository = customerRepository; _mapper = mapper; } public async Task AddCustomerAsync(CustomerDto customerDto) { var customerEntity = _mapper.Map<Customer>(customerDto); _customerRepository.AddCustomer(customerEntity); await _customerRepository.SaveChangesAsync(); }}
- CustomerRepository:
public class CustomerRepository : ICustomerRepository { private readonly DbContext _context; public CustomerRepository(DbContext context) { _context = context ?? throw new ArgumentNullException(nameof(context)); } public void AddCustomer(Customer customer) { if (customer == null) { throw new ArgumentNullException(nameof(customer)); } _context.Add(customer); } public async Task<bool> SaveChangesAsync() { return (await _context.SaveChangesAsync() > 0); } }
- ValidationAttribute:
public class RequiredIfAttribute : ValidationAttribute { private readonly string _otherPropertyName; public RequiredIfAttribute(string otherPropertyName) { _otherPropertyName = otherPropertyName; } protected override ValidationResult IsValid(object value, ValidationContext validationContext) { var containerType = validationContext.ObjectInstance.GetType(); var dependentProperty = containerType.GetProperty(_otherPropertyName); if (dependentProperty == null) { return new ValidationResult($"Unknown property: {_otherPropertyName}"); } var dependentValue = dependentProperty.GetValue(validationContext.ObjectInstance, null); if (IsNullOrEmpty(value) && IsNullOrEmpty(dependentValue)) { return new ValidationResult( $"Either '{validationContext.MemberName}' or '{_otherPropertyName}' must be provided."); } return ValidationResult.Success; } private bool IsNullOrEmpty(object value) { if (value == null) return true; if (value is string str) return string.IsNullOrWhiteSpace(str); return false; } }
And last the dataSet entity:
[Table("Customers")] public class Customer { [Key] public int Id { get; set; } [Required] [MaxLength(50)] public string Name { get; set; } [Required] [MaxLength(50)] public string SurName { get; set; } [Required] [Unicode(false)] [MaxLength(10)] public string Reference { get; set; } [Column(TypeName = "datetime2")] public DateTime? DateOfBirth { get; set; } [Column(TypeName = "nvarchar(250)")] public string? Email { get; set; } } }
1 Answer 1
Minor Issues
Formatting:
I presume that the code rendering here on site does not match what you have in your IDE. Quoted code blocks are already annoying with the SE editor, quoted code blocks under list elements are even more annoying.
That said I still want to point out that the formatting you have there is not entirely consistent around the use of empty lines.
I would warmly recommend using automated tooling to enforce coding conventions for you, whether that is setting up a simple .editorconfig
with the dotnet rules and enabling style analysis in your project settings or going all the way with StyleCop.Analyzers
.
I like seeing defensive programming like the CustomerRepository
constructor, but I was thrown a bit by the throw new ArgumentNullException
not being indented to quickly signal it's a line-continuation.
asnyc
niceties:
Using async
is generally a good idea. When declaring async methods, it is generally encouraged to expose a CancellationToken cancellationToken = null
parameter at the end to allow cancellation token forwarding.
More advanced code analyzers will then suggest forwarding the cancellation token through the call tree to allow for painless and quick task cancellation.
Framework and Compiler wins
The C# framework and especially the compiler can take care of a lot of work for you. One of the things you probably want is the use of nullable
reference type support. I strongly encourage you to enable the nullable support for all your projects. If you want to rely on the compiler explicitly, you can even enable WarningsAsErrors
. I personally want the compiler to yell at me when I'm doing nonsense, but not everybody likes that ;)
Speaking of nullable type support: Your Customer
entity is using that for DoB and Email. Speaking of those: C# has support for a DateOnly
type. I want to strongly suggest using that for the Dto as well as the entity. All good relational DB engines these days support sql native data types that are equivalent to DateOnly.
Validation
I don't see the validation for the DoB implying an age of 18 years or more.
In addition I would change the name for RequiredIf
to RequiredUnless
.
That seems to better reflect the actual validation rule. A RequiredIf would probably instead have the following if-condition in the IsValid
:
if (!IsNullOrEmpty(value) || IsNullOrEmpty(triggerValue))
// i.e. (triggerValue.IsNullOrEmpty && value.IsNullOrEmpty) || !value.IsNullOrEmpty
// or formulated differently: if value is nullorempty, then triggervalue also is nullorempty
_context = context ?? throw new ArgumentNullException(nameof(context));
in your repository layer constructor but not in your service layer nor your controller. I encourage the "fail fast" philosophy to guarantee your dependencies are valid before you have to use them. 2. Any classes you do not have an intentional design for inheritance should be markedsealed
. 3. Your asynchronous methods should take/pass an optional cancellation token. And 4. UseConfigureAwait(false)
on the end of async calls. \$\endgroup\$