Things to look in PR!!!

Some Common Review Points

Proper use of REST verbs

use of httpPost instead of httpPut

Proper API routes

‘/api/abc/create’ should be ‘/api/abc’

Common route prefix

like 'api' in each route should move to common location

Use of Linq

Using linq clause after fetching records from DB

Transactions

Use of Transaction in multiple database calls

Dependency Injection

Proper and intelligent use of dependency injection

services.AddScoped<IOperationScoped, Operation>();

services.AddTransient<IOperationTransient, Operation>(); services.AddSingleton<IOperationSingleton, Operation>();

Thin Controller

Business logic shouldn’t be in the Controller

Thin Controller

Repositories should be accessed by the Service and not to be called in the Controllers

[Route("api/Practice")]
[Authorize]
public class PracticeController : BaseController
{
    public readonly IPracticeRepository repository;

    public PracticeController(IPracticeRepository repository)
    {
        this.repository = repository;
    }

    [HttpPut]
    [SwaggerResponse(200, Type = typeof(DataTransferObject<PracticeDTO>))]
    public async Task<IActionResult> Update([FromBody]PracticeDTO dtoObject)
    {
        return JsonResponse(this.repository.Update(dtoObject.ConvertToEntity()));
    }
}

Async Await

“Task” should be returned to the parent instead of awaiting through the complete call stack unless the value returned is required to be used right away

Method

Method names should be valid verbs and not lengthy sentences

public async Task<PagedList<InspectionAnswerQuestion>> GetAnswerByInspectionIdWithPagedListAndSorting(long id, int pageNumber, int pageSize, string sortExpression)
{
    string query = "where InspectionId=" + id;
    var totalRecords = await _simpleRepository.RecordCountAsync<InspectionAnswerQuestion>(query);
    if (totalRecords < 1)
    {
        return new PagedList<InspectionAnswerQuestion>();
    }

    var records = await _simpleRepository.GetListPagedAsync<InspectionAnswerQuestion>(pageNumber, pageSize, query, sortExpression);

    return new PagedList<InspectionAnswerQuestion>(records, totalRecords);
}

Method

Avoid more than four parameters in function

public async Task<PagedList<QuestionView>> GetQuestion(int? buildingId, int? buildingTypeId, int questionId, int pageNumber, int pageSize, string sortExpression)

Method

Avoid long methods

public async Task<DataTransferObject<string>> ImportCSV(JSONAPIRequest request)
{
    var response = new DataTransferObject<string>();
    List<RecommendationEngineSpreadsheetDTO> dataList;
    List<RegimenRecommendationDTO> regimenRecommendationDTOs = new List<RegimenRecommendationDTO>();
    HashSet<MedicationDTO> medicationDTOs = new HashSet<MedicationDTO>();
    HashSet<DosageDTO> dosageDTOs = new HashSet<DosageDTO>();
    HashSet<FrequencyDTO> frequencyDTOs = new HashSet<FrequencyDTO>();
    HashSet<LabTestDTO> labTestDTOs = new HashSet<LabTestDTO>();
    string filePath = recommendationEngineConfig.FilePath;
    if (filePath == null)
    {
        response.HasErrors = true;
        response.Error = new Exception("File Path not defined in configuration");
        return response;
    }
    if (!File.Exists(filePath))
    {
        response.HasErrors = true;
        response.Error = new Exception(ErrorStrings.FileNotFound + filePath);
        return response;
    }
    using (TextReader fileReader = File.OpenText(filePath))
    {
        CsvReader csv = new CsvReader(fileReader);
        dataList = csv.GetRecords<RecommendationEngineSpreadsheetDTO>().ToList();
    }

    foreach (var elem in dataList)
    {
        var gender = elem.Gender == "F" ? (int)Gender.Female : (int)Gender.Male;
        MedicationDTO medication1 = new MedicationDTO(elem.Treatment1, elem.Treatment1_Instructions);
        DosageDTO dosage1 = new DosageDTO(elem.Treatment1_Quantity, elem.Units);
        FrequencyDTO frequency1 = new FrequencyDTO(elem.Treatment1_Frequency);
        regimenRecommendationDTOs.Add(new RegimenRecommendationDTO(elem.Test, elem.Range_Compare, gender, elem.Reference_Range_Low, elem.Reference_Range_High, elem.Units, elem.P1_Percentage, elem.P_Range_1_Low, elem.P_Range_1_High, elem.Notes, dosage1, frequency1, medication1, this.labTestRepository));
        medicationDTOs.Add(medication1);
        frequencyDTOs.Add(frequency1);
        dosageDTOs.Add(dosage1);
        MedicationDTO medication2 = new MedicationDTO(elem.Treatment2, elem.Treatment2_Instructions);
        DosageDTO dosage2 = new DosageDTO(elem.Treatment2_Quantity, elem.Units);
        FrequencyDTO frequency2 = new FrequencyDTO(elem.Treatment2_Frequency);
        regimenRecommendationDTOs.Add(new RegimenRecommendationDTO(elem.Test, elem.Range_Compare, gender, elem.Reference_Range_Low, elem.Reference_Range_High, elem.Units, elem.P2_Percentage, elem.P_Range_2_Low, elem.P_Range_2_High, elem.Notes, dosage2, frequency2, medication2, this.labTestRepository));
        medicationDTOs.Add(medication2);
        frequencyDTOs.Add(frequency2);
        dosageDTOs.Add(dosage2);
        MedicationDTO medication3 = new MedicationDTO(elem.Treatment3, elem.Treatment3_Instructions);
        DosageDTO dosage3 = new DosageDTO(elem.Treatment3_Quantity, elem.Units);
        FrequencyDTO frequency3 = new FrequencyDTO(elem.Treatment3_Frequency);
        regimenRecommendationDTOs.Add(new RegimenRecommendationDTO(elem.Test, elem.Range_Compare, gender, elem.Reference_Range_Low, elem.Reference_Range_High, elem.Units, elem.P3_Percentage, elem.P_Range_3_Low, elem.P_Range_3_High, elem.Notes, dosage3, frequency3, medication3, this.labTestRepository));
        medicationDTOs.Add(medication3);
        frequencyDTOs.Add(frequency3);
        dosageDTOs.Add(dosage3);
        MedicationDTO medication4 = new MedicationDTO(elem.Treatment4, elem.Treatment4_Instructions);
        DosageDTO dosage4 = new DosageDTO(elem.Treatment4_Quantity, elem.Units);
        FrequencyDTO frequency4 = new FrequencyDTO(elem.Treatment4_Frequency);
        regimenRecommendationDTOs.Add(new RegimenRecommendationDTO(elem.Test, elem.Range_Compare, gender, elem.Reference_Range_Low, elem.Reference_Range_High, elem.Units, elem.P4_Percentage, elem.P_Range_4_Low, elem.P_Range_4_High, elem.Notes, dosage4, frequency4, medication4, this.labTestRepository));
        medicationDTOs.Add(medication4);
        frequencyDTOs.Add(frequency4);
        dosageDTOs.Add(dosage4);
        MedicationDTO medication5 = new MedicationDTO(elem.Treatment5, elem.Treatment5_Instructions);
        DosageDTO dosage5 = new DosageDTO(elem.Treatment5_Quantity, elem.Units);
        FrequencyDTO frequency5 = new FrequencyDTO(elem.Treatment5_Frequency);
        regimenRecommendationDTOs.Add(new RegimenRecommendationDTO(elem.Test, elem.Range_Compare, gender, elem.Reference_Range_Low, elem.Reference_Range_High, elem.Units, elem.P5_Percentage, elem.P_Range_5_Low, elem.P_Range_5_High, elem.Notes, dosage5, frequency5, medication5, this.labTestRepository));
        medicationDTOs.Add(medication5);
        frequencyDTOs.Add(frequency5);
        dosageDTOs.Add(dosage5);
        if (!labTestDTOs.Any(e => e.Name == elem.Test)) {
            LabTestDTO labTestDTO = new LabTestDTO(elem.Range_Compare, elem.Test);
            labTestDTOs.Add(labTestDTO);
        }
    }
    foreach (var medication in medicationDTOs)
    {
        DataTransferObject<MedicationDTO> dbMedication = await medicationService.CreateAsync(medication);
        medication.Id = dbMedication.Result.Id;
    }

    foreach (var frequency in frequencyDTOs)
    {
        DataTransferObject<FrequencyDTO> dbFrequency = await frequencyService.CreateAsync(frequency);
        frequency.Id = dbFrequency.Result.Id;
    }

    foreach (var dosage in dosageDTOs)
    {
        DataTransferObject<DosageDTO> dbDosage = await dosageService.CreateAsync(dosage);
        dosage.Id = dbDosage.Result.Id;
    }

    foreach (var labTest in labTestDTOs)
    {
        this.labTestRepository.Create(new LabTest
        {
            CreatedBy = 1,
            CreatedOn = DateTime.UtcNow,
            CompareWith = labTest.CompareWith,
            Name = labTest.Name
        });
    }
    await this._unitOfWork.SaveAsync();

    foreach (var recommendation in regimenRecommendationDTOs)
    {
        MedicationDTO medicationDTO = medicationDTOs.First(x => x.Equals(recommendation.Medication));
        DosageDTO dosageDTO = dosageDTOs.First(x => x.Equals(recommendation.Dosage));
        FrequencyDTO frequencyDTO = frequencyDTOs.First(x => x.Equals(recommendation.Frequency));
        recommendation.FrequencyId = frequencyDTO.Id;
        recommendation.Frequency = frequencyDTO;
        recommendation.MedicationId = medicationDTO.Id;
        recommendation.Medication = medicationDTO;
        recommendation.DosageId = dosageDTO.Id;
        recommendation.Dosage = dosageDTO;
    }
    var result = await base.CreateAsync(regimenRecommendationDTOs);
    return new DataTransferObject<string>(SucessStrings.Success);
}

Authentication

Add proper role authentication

Throw everywhere

 

Error handling shouldn’t be implemented repeatedly (global exception handling)

Keys

 

Do not store keys in code or in files on source control

Constants

Avoid writing messages directly under the code, use proper constants for it

public async Task<Question> UpdateQuestion(Question question)
{
    if (question == null)
    {
        throw new ArgumentNullException($"Object cannot update a null value", nameof(question));
    }

    if (question.Id < 1)
    {
        throw new ArgumentNullException($"The Id must be greater than 0", nameof(question.Id));
    }

    try
    {
        await _simpleRepository.UpdateAsync<Question>(question);
    }
    catch (Exception ex)
    {
        throw ex;
    }
    return question;
}

.Net Recipe specific review points

Request Info

Proper use of request info

Base Classes

Proper use of base classes (like GetCount())

public class FitbitDetailRepository : AuditableRepository<FitbitDetail, long>, IFitbitDetailRepository
{
    public FitbitDetailRepository(IRequestInfo requestInfo) : base(requestInfo)
    {
    }

    public async Task<int> GetFitbitCount()
    {
        return (await base.GetAll(null)).Count();
    }
}

Database Context

UnitOfWork and dbContext

using (var trans = this.UnitOfWork.DBContext.Database.BeginTransaction())
{
    IEnumerable<ApplicationUser> users = await this.UnitOfWork.UserRepository.GetByDistributorId(distributorIds);
    IList<UserSalaryPolicyGroupDTO> dtoObjects = await this.SaveAsync(userSalaryPolicyGroupId, users);

    await this.UnitOfWork.SaveAsync();
    trans.Commit();

    return dtoObjects;
}
using (var trans = (new LagunContext()).Database.BeginTransaction())
{
    IEnumerable<ApplicationUser> users = await this.UnitOfWork.UserRepository.GetByDistributorId(distributorIds);
    IList<UserSalaryPolicyGroupDTO> dtoObjects = await this.SaveAsync(userSalaryPolicyGroupId, users);

    await this.UnitOfWork.SaveAsync();
    trans.Commit();

    return dtoObjects;
}

Query

Difference between ‘DefaultListQuery’ and ‘DefaultSingleQuery’ and its usage

public async Task<ApplicationUser> GetByUserEmail(string email)
{
    return await this.DBContext.Set<ApplicationUser>().Where(x => x.Email == email).SingleOrDefaultAsync();
}
public async Task<ApplicationUser> GetByUserEmail(string email)
{
    return await this.DefaultSingleQuery.Where(x => x.Email == email).SingleOrDefaultAsync();
}
public async Task<IEnumerable<ApplicationUser>> GetAllStaffAsync(JSONAPIRequest request)
{
    return await this.DefaultListQuery
        .Include(x => x.JobTitle)
        .Where(x => x.UserType == (int)UserRole.Staff || x.UserType == (int)UserRole.Doctor)
        .GenerateQuery(request)
        .ToListAsync();
}
public async Task<IEnumerable<ApplicationUser>> GetAllStaffAsync(JSONAPIRequest request)
{
    return await this.DBContext.Set<ApplicationUser>()
        .Include(x => x.JobTitle)
        .Where(x => x.UserType == (int)UserRole.Staff || x.UserType == (int)UserRole.Doctor)
        .GenerateQuery(request)
        .ToListAsync();
}

Audit

Usage of AuditableRepository (do not fill createdBy and updatedBy)

public override ApplicationUser Create(ApplicationUser entity)
{
    entity.CreatedBy = RequestInfo.UserId;
    entity.CreatedOn = DateTime.UtcNow;
    entity.LastModifiedBy = RequestInfo.UserId;
    entity.LastModifiedOn = DateTime.UtcNow;

    return base.Create(entity);
}
public override ApplicationUser Update(ApplicationUser entity)
{
    entity.LastModifiedBy = RequestInfo.UserId;
    entity.LastModifiedOn = DateTime.UtcNow;

    return base.Update(entity);
}
public override async Task DeleteAsync(long id)
{
    var entity = await GetAsync(id);
    if (entity != null)
    {
        entity.LastModifiedOn = DateTime.UtcNow;
        entity.LastModifiedBy = this.RequestInfo.UserId;
        entity.IsDeleted = true;
        base.Update(entity);
    }

    await base.DeleteAsync(id);
}

Audit

Add ‘AuditOperationAttribute’ over every new created method

[AuditOperationAttribute(OperationType.Read)]
Task<TEntity> GetAsync(TKey id);

[AuditOperationAttribute(OperationType.Read)]
Task<IEnumerable<TEntity>> GetAsync(IList<TKey> ids);

[AuditOperationAttribute(OperationType.Read)]
Task<TEntity> GetEntityOnly(TKey id);

[AuditOperationAttribute(OperationType.Read)]
Task<int> GetCount();

[AuditOperationAttribute(OperationType.Read)]
Task<IEnumerable<TEntity>> GetAll(JSONAPIRequest request);

[AuditOperationAttribute(OperationType.Create)]
TEntity Create(TEntity entity);

[AuditOperationAttribute(OperationType.Update)]
TEntity Update(TEntity entity);

[AuditOperationAttribute(OperationType.Delete)]
Task DeleteAsync(TKey id);

Tenancy In

.Net Recipe

Implementation

  • Make a layer for handling tenancy

  • Don’t write TenantID where ever it needs being saved or fetched

  • Save the current TenantID in RequestInfo so you don’t have to fetch it each time you fetch, save or update a record

  • Tenant layer should inherit all base classes and default query methods

  • All other repositories and entities should inherit the tenant layer

Implementation

Tenant Entity

public class TenantEntity<TKey, TTenant> : EntityBase<TKey>
{
    public TTenant TenantId { get; set; }
}
public class Practice : TenantEntity<long, int>
{
    [Required]
    [MaxLength(100)]
    public string CompanyInfo { get; set; }

    [Required]
    [MaxLength(50)]
    public string PhoneNumber { get; set; }

    [Required]
    [EmailAddress(ErrorMessage = "Email is not valid")]
    public string EmailAddress { get; set; }

    public long CityId { get; set; }
}

Implementation

Request Info

public int TenantId
{
    get
    {
        int defaultId = 0;
        if (HttpContext.Current != null && HttpContext.Current.Request.Headers["TenantId"] != null)
        {
            int.TryParse(HttpContext.Current.Request.Headers["TenantId"].ToString(), out defaultId);
        }
        else
        {
            int.TryParse(this.GetValueFromClaims(General.ClaimTenantId), out defaultId);
        }

        return defaultId;
    }
}

Implementation

Tenant Repository

public class TenantRepository<TEntity, TKey> : AuditableRepository<TEntity, TKey>
        where TEntity : TenantEntity<TKey, long>
        where TKey : IEquatable<TKey>
{
    public TenantRepository(IRequestInfo requestInfo)
        : base(requestInfo)
    {
    }

    protected override IQueryable<TEntity> DefaultListQuery
    {
        get
        {
            return base.DefaultListQuery.Where(x => x.TenantId == RequestInfo.TenantId);
        }
    }

    protected override IQueryable<TEntity> DefaultSingleQuery
    {
        get
        {
            return base.DefaultSingleQuery.Where(x => x.TenantId == RequestInfo.TenantId);
        }
    }
}

Questions???

Things to look in PR

By Tahir Hassan

Made with Slides.com

Things to look in PR

  • 196

More from Tahir Hassan