1
\$\begingroup\$

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");
        }
    }
}
\$\endgroup\$
0

2 Answers 2

5
\$\begingroup\$

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

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
  • 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
\$\endgroup\$
0
2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks so much, I really appreciate your comments i will refactor them straight away \$\endgroup\$
    – CodeRealm
    Feb 4, 2022 at 19:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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