diff --git a/Security/Service/Implementations/AuthService.cs b/Security/Service/Implementations/AuthService.cs index 107565f..8af0eaa 100644 --- a/Security/Service/Implementations/AuthService.cs +++ b/Security/Service/Implementations/AuthService.cs @@ -20,7 +20,7 @@ public class AuthService : IAuthService private readonly UserManager _userManager; private readonly ITokenGenerator _tokenGenerator; private readonly IEmailService _emailService; - private readonly IHttpContextAccessor _httpContextAccessor; + private readonly IHttpContextAccessor _httpContextAccessor;//not needed private readonly ICurrentUserService _currentUserService; private readonly IWebHostEnvironment _webHostEnvironment; private readonly MailTemplatesConstants _mailTemplatesConstants; diff --git a/TestingApi/Controllers/AnswerTemplatesController.cs b/TestingApi/Controllers/AnswerTemplatesController.cs index 09e355e..71ff5ef 100644 --- a/TestingApi/Controllers/AnswerTemplatesController.cs +++ b/TestingApi/Controllers/AnswerTemplatesController.cs @@ -15,6 +15,7 @@ public class AnswerTemplatesController : ControllerBase private readonly IQuestionTmplService _questionTmplService; private readonly ILogger _logger; + //logger, questionService and answerService are not needed. also very long string, better to split into several lines public AnswerTemplatesController(IQuestionService questionService, ILogger logger, IAnswerService answerService, IAnswerTmplService answerTmplService, IQuestionTmplService questionTmplService) { _logger = logger; diff --git a/TestingApi/Data/DataContext.cs b/TestingApi/Data/DataContext.cs index b50f96e..266d249 100644 --- a/TestingApi/Data/DataContext.cs +++ b/TestingApi/Data/DataContext.cs @@ -69,7 +69,7 @@ private void OnBeforeSaving() switch (entry.State) { case EntityState.Modified: - baseEntity.ModifiedTimestamp = DateTime.Now; + baseEntity.ModifiedTimestamp = DateTime.Now;//better use utc time. it may cause data inconsistency after deployment to many availability zones if(_currentUserService.UserId != null) baseEntity.ModifiedBy = Guid.Parse(_currentUserService.UserId); break; diff --git a/TestingApi/Dto/FiltersDto.cs b/TestingApi/Dto/FiltersDto.cs index 7c06b22..a829ec7 100644 --- a/TestingApi/Dto/FiltersDto.cs +++ b/TestingApi/Dto/FiltersDto.cs @@ -6,7 +6,7 @@ public class FiltersDto { public string? SearchTerm { get; set; } public string? SortColumn { get; set; } - public string? SortOrder { get; set; } + public string? SortOrder { get; set; }//it can be boolean [Range(1, int.MaxValue, ErrorMessage = "Page must be greater than 0")] public int Page { get; set; } diff --git a/TestingApi/Extensions/RegisterServices.cs b/TestingApi/Extensions/RegisterServices.cs index c2d40c7..bb1a534 100644 --- a/TestingApi/Extensions/RegisterServices.cs +++ b/TestingApi/Extensions/RegisterServices.cs @@ -62,7 +62,7 @@ public static void ConfigureAuth(this IServiceCollection services, IConfiguratio ValidateIssuerSigningKey = true, ClockSkew = TimeSpan.Zero, - IssuerSigningKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(configuration["Auth:SecretKey"])), + IssuerSigningKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(configuration["Auth:SecretKey"])),//better to get the config model. this is safer }; }); } diff --git a/TestingApi/Services/Implementations/AnswerService.cs b/TestingApi/Services/Implementations/AnswerService.cs index 6901c8a..eb22f4f 100644 --- a/TestingApi/Services/Implementations/AnswerService.cs +++ b/TestingApi/Services/Implementations/AnswerService.cs @@ -20,7 +20,8 @@ public AnswerService(DataContext dataContext, ILogger logger, IMa _logger = logger; } - public async Task GetAnswerById(Guid id, CancellationToken cancellationToken = default) + public async Task GetAnswerById(Guid id, CancellationToken cancellationToken = default)//no need to add word 'Answer' into the method name. + //it is already AnswerService { var answers = await _dataContext.Answers .FirstOrDefaultAsync(a => a.Id == id, cancellationToken); @@ -28,7 +29,8 @@ public AnswerService(DataContext dataContext, ILogger logger, IMa return _mapper.Map(answers); } - public async Task AnswerExistsAsync(Guid id, CancellationToken cancellationToken = default) + public async Task AnswerExistsAsync(Guid id, CancellationToken cancellationToken = default)//looks like you need a generic repo to put common logic here + //I see at least several very similar methods in most of the services { return await _dataContext.Answers.AnyAsync(a => a.Id == id, cancellationToken); } diff --git a/TestingApi/Services/Implementations/FileService.cs b/TestingApi/Services/Implementations/FileService.cs index 4ed4237..fdc6145 100644 --- a/TestingApi/Services/Implementations/FileService.cs +++ b/TestingApi/Services/Implementations/FileService.cs @@ -9,11 +9,13 @@ public class FileService : IFileService { private readonly IConfiguration _configuration; - public FileService(IConfiguration configuration) + public FileService(IConfiguration configuration)//do not inject configuration. inject model instead { _configuration = configuration; } - public async Task RemoveFilesByNameIfExistsAsync(string fileName, CancellationToken cancellationToken = default) + public async Task RemoveFilesByNameIfExistsAsync(string fileName, CancellationToken cancellationToken = default)//there is no need in this method + //old file is replaced by new one when created + //read FileMode.Create description { var folderName = _configuration["FileStorage:FolderPath"]; var pathToRemoveFiles = Path.Combine(Directory.GetCurrentDirectory(), folderName); diff --git a/TestingApi/Services/Implementations/TestStatisticService.cs b/TestingApi/Services/Implementations/TestStatisticService.cs index c6e90b4..c46999e 100644 --- a/TestingApi/Services/Implementations/TestStatisticService.cs +++ b/TestingApi/Services/Implementations/TestStatisticService.cs @@ -19,7 +19,7 @@ public TestStatisticService(DataContext dataContext, IMapper mapper) _mapper = mapper; } - public async Task GetTestStatistic(Guid testId, + public async Task GetTestStatistic(Guid testId,//lines 29, 32, 36, 47 calls can be done in parallel CancellationToken cancellationToken = default) { var test = await _dataContext.Tests.Where(t => t.Id == testId).FirstAsync(cancellationToken); diff --git a/TestingApi/Services/Implementations/UserService.cs b/TestingApi/Services/Implementations/UserService.cs index 8395032..f067ef7 100644 --- a/TestingApi/Services/Implementations/UserService.cs +++ b/TestingApi/Services/Implementations/UserService.cs @@ -76,7 +76,8 @@ public async Task UserExistsAsync(Guid id, CancellationToken cancellationT .AnyAsync(e => e.Id == id, cancellationToken); } - public async Task RegisterUserAsync(UserSignUpDto userSignUpDto, + public async Task RegisterUserAsync(UserSignUpDto userSignUpDto,//this way of handling users is not the best + //better to make security api visible to FE and do all the call directly CancellationToken cancellationToken = default, bool isAdmin = false) { var jsonContent = JsonSerializer.Serialize(userSignUpDto); diff --git a/TestingApi/Services/Implementations/UserTestService.cs b/TestingApi/Services/Implementations/UserTestService.cs index 156ff3f..f6a6b58 100644 --- a/TestingApi/Services/Implementations/UserTestService.cs +++ b/TestingApi/Services/Implementations/UserTestService.cs @@ -202,7 +202,7 @@ public async Task GetUserTestResults(Guid userId, Guid te }; } - private IQueryable GetAnsweredUserQuestionResultQuery(Guid userId, Guid testId) + private IQueryable GetAnsweredUserQuestionResultQuery(Guid userId, Guid testId)//very big linq. better to split into private methods { var userAnswersDetailsQuery = _dataContext.UserAnswers .Include(ua => ua.Question)