Update person name in database

Question

Can someone review this implemented solution and provide commentary the code is working well but will need refactor and improve this code.

using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Configuration;
using System;

namespace PersonUpdateApi.Controllers
{
    public class PersonUpdateModel
    {
        public int Id { get; set; }
        public string NewFirstname { get; set; }
        public string NewSurname { get; set; }
    }

    [ApiController]
    [Route("[controller]")]
    public class PersonController : ControllerBase
    {
        private readonly IConfiguration _config;
        private Logger _logger;

        public PersonController(IConfiguration config)
        {
            _config = config;
        }

        public void LogUpdateRequest(PersonUpdateModel p)
        {
            _logger.Info($"Update request received. Person ID: {p.Id}, New forename = {p.NewFirstname}, New surname = {p.NewSurname}");
        }

        private void LogSuccessfulUpdate(PersonUpdateModel p)
        {
            _logger.Info($"Update request was successful for Person ID: {p.Id}.");
        }

        private void LogException(PersonUpdateModel p, Exception ex)
        {
            _logger.Info($"An error occurred when updating the person. Person ID: {p.Id}, New forename = {p.NewFirstname}, New surname = {p.NewSurname}", ex);
        }

        [HttpPost]
        public IActionResult Update(PersonUpdateModel p)
        {
            _logger = new Logger();
            LogUpdateRequest(p);

            if (string.IsNullOrWhiteSpace(p.NewFirstname))
                return BadRequest("There was no firstname specified");

            if (string.IsNullOrWhiteSpace(p.NewSurname))
                return BadRequest("There was no surname specified");

            if (p.Id < 1)
                return BadRequest("Invalid person ID");

            string dbConnectionStr = _config.GetValue<string>("Database:ConnectionString");
            int dbTimeoutInSeconds = 120;

            try
            {
                string updateSql = "UPDATE People SET Firstname = '" + p.NewFirstname + "', Surname = '" + p.NewSurname + "' WHERE PersonId = " + p.Id;

                Database db = new Database();
                db.ExecuteNonReturningQuery(dbConnectionStr, updateSql, dbTimeoutInSeconds);

            }
            catch(Exception ex)
            {
                LogException(p, ex);
                return BadRequest("There was an error updating the person in the database. Please try again later.");
            }

            LogSuccessfulUpdate(p);
            return Ok("The update was successful");
        }
    }
}

Answer 1

PersonUpdateModel

• Depending on how sophisticated your Person model is, you might need to consider to extend your update model with middle name
• Prefixing first name and surname with new is unnecessary
  • since you don't have other properties with old or current
• Using a consistent naming help eligibility
  • first name and last name
  • forename and surname
  • given name and family name

PersonController

ctor

• Most of the time a class-wide logger is the preferred approach
• If you register your Logger inside your DI then you can receive an instance of it via the constructor

LogXYZ

• These methods (most probably) should not need to be public

LogUpdateRequest

• This can be extracted into a request logger middleware

LogSuccessfulUpdate

• Most of the time the successful operation logs are pointless
  • You perform logging primarily to help yourself during debugging (so the exceptional cases are the interesting one)
  • If you want to use the successful logs to calculate success-failure ratio then consider to introduce two counters instead

LogException

• Info might not be the best severity level here
  • Warning or Error might be more suitable

Update

• Naming a parameter to p is not really a good idea
  • try to find more expressive name
• Your p variable might be null if you receive a malformed request body
  • You should check whether or not it is null before you try to access any of its member otherwise it will throw NullReferenceException
  • The request will fail miserable inside your LogUpdateRequest method, so you will not have any log message about it
• Retrieving the connection string all the time most probably not needed
  • If it does not change dynamically then you can retrieve that value inside the constructor for example
• dbTimeoutInSeconds could be marked as const
• Performing database operation from the controller is not really a good practice
  • Try to introduce layers, like service and repository each with its own responsibility
• updateSql: string concatenation of sql DML is prone to SQL Inject
  • The preferred way is to use command parameters
• Creating a Database instance manually might not be the best approach
  • You should consider to register that as well inside the DI
• Returning with BadRequest in case of exception is misleading
  • StatusCode(StatusCodes.Status500InternalServerError) might be more suitable
  • Please do not pass the ex to the StatusCode method call, because it might reveal system internals which can be valuable for malicious users

Answer 2

Quick remarks (in addition to Peter Csala's review):

• Your controller does too much. Keep it thin and lean, and create other classes to implement the logic etc. For instance, use MediatR.

  Right now your controller is 80-something lines and yet there is only one actual method doing anything; and honestly it is doing something simple. Your approach will become unmaintainable once you need to apply complicated business logic.

• Don't create a method to log a single line unless that method will be used in many places. (And even then there are often better solutions.)

• Use Dapper instead of ADO.NET.