Code Review Suggestions for CostItemsController

Question:

Проверь мой код ''' using AutoMapper; using BuildingFlow.Data.Contract; using BuildingFlow.Data.Entities; using BuildingFlow.Services.Models.ViewModels; using BuildingFlow.Services.Models.ViewModels.ProjectStage; using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using OfficeOpenXml;

namespace BuildingFlow.Web.Controllers;

public class CostItemsController : Controller { private readonly IUnitOfWork _unitOfWork; private readonly IMapper _mapper; private readonly IRepository<CostItem> _costItemRepository;

public CostItemsController(IUnitOfWork unitOfWork, IMapper mapper) { _unitOfWork = unitOfWork; _mapper = mapper; _costItemRepository = _unitOfWork.GetRepository<CostItem>(); }

// GET: CostItems public async Task<IActionResult> Index() { var costItems = await _costItemRepository.AsReadOnlyQueryable() .Where(ci => ci.ProjectId == 1) .ToListAsync();

var costItemViewModel = _mapper.Map<IEnumerable<CostItemViewModel>>(costItems);

return View(costItemViewModel);

}

//GET: CostItems/Upload [HttpGet] public IActionResult Upload() { return View(); }

[HttpPost] public async Task<IActionResult> Upload(IFormFile file) { if (file == null || file.Length == 0) { ModelState.AddModelError("", "Please upload a file."); return View(); }

var costItems = new List<CostItemAddViewModel>();

using (var stream = new MemoryStream())
{
  await file.CopyToAsync(stream);
  ExcelPackage.LicenseContext = LicenseContext.NonCommercial;
  using (var package = new ExcelPackage(stream))
  {
    var worksheet = package.Workbook.Worksheets.First();
    var rowCount = worksheet.Dimension.Rows;

    for (int row = 2; row <= rowCount; row++) // Начинаем со второй строки, чтобы пропустить заголовки
    {
      var costItem = new CostItemAddViewModel();

      costItem.NumberItem =
        string.IsNullOrWhiteSpace(worksheet.Cells[row, 2].Text) ? null : worksheet.Cells[row, 2].Text;
      costItem.NodeCode = string.IsNullOrWhiteSpace(worksheet.Cells[row, 3].Text)
        ? null
        : worksheet.Cells[row, 3].Text;
      costItem.CostName = string.IsNullOrWhiteSpace(worksheet.Cells[row, 4].Text)
        ? null
        : worksheet.Cells[row, 4].Text;
      costItem.ISRComment =
        string.IsNullOrWhiteSpace(worksheet.Cells[row, 5].Text) ? null : worksheet.Cells[row, 5].Text;
      costItem.ContractorComment = string.IsNullOrWhiteSpace(worksheet.Cells[row, 6].Text)
        ? null
        : worksheet.Cells[row, 6].Text;
      costItem.UnitOfMeasure = string.IsNullOrWhiteSpace(worksheet.Cells[row, 7].Text)
        ? null
        : worksheet.Cells[row, 7].Text;
      costItem.ConsumptionCoefficient = ParseNullableDecimal(worksheet.Cells[row, 8].Text);
      costItem.BOCount = ParseNullableInt(worksheet.Cells[row, 9].Text);
      costItem.TotalCount = ParseNullableDecimal(worksheet.Cells[row, 10].Text);
      costItem.MatEquipmPrice = ParseNullableDecimal(worksheet.Cells[row, 11].Text);
      costItem.SmpPnpPrice = ParseNullableDecimal(worksheet.Cells[row, 12].Text);
      costItem.TotalPrice = ParseNullableDecimal(worksheet.Cells[row, 13].Text);
      //MatEquipmCost = ParseNullableDecimal(worksheet.Cells[row, 14].Text),
      //SmpPnpCost = ParseNullableDecimal(worksheet.Cells[row, 15].Text),
      //TotalCost = ParseNullableDecimal(worksheet.Cells[row, 16].Text),
      costItem.ProjectId = 1;
      

      // Определение типа записи
      costItem.TypeCost = DetermineType(costItem.NodeCode, costItem.UnitOfMeasure);

      if (costItem.TypeCost == "Материал")
      {
        costItem.MatEquipmCost = CalculateMatEquipmCost(costItem.TotalCount, costItem.MatEquipmPrice);
      }

      if (costItem.TypeCost == "Работа")
      {
        costItem.SmpPnpCost = CalculateSmpPnpCost(costItem.TotalCount, costItem.SmpPnpPrice);
      }

      costItems.Add(costItem);
    }

    // Построение иерархии
    var hierarchicalCostItems = BuildHierarchy(costItems);

    // Вычисление затрат
    CalculateCosts(hierarchicalCostItems);

    // Преобразование в плоский список
    var flatCostItems = FlattenHierarchy(hierarchicalCostItems).ToList();

    // Маппинг в сущности
    var entities = _mapper.Map<List<CostItem>>(flatCostItems);

    // Сохранение записей без ParentId
    _costItemRepository.CreateRange(entities);
    await _unitOfWork.SaveChangesAsync();

    // Создание словаря NumberItem -> Id
    var numberItemToId = await _costItemRepository.AsReadOnlyQueryable()
      .Where(ci => ci.ProjectId == 1)
      .ToListAsync();

    var mapping = numberItemToId.ToDictionary(ci => ci.NumberItem, ci => ci.Id);

    // Обновление ParentId
    foreach (var entity in numberItemToId)
    {
      var parentNumberItem = GetParentNumberItem(entity.NumberItem);
      if (!string.IsNullOrEmpty(parentNumberItem) && mapping.ContainsKey(parentNumberItem))
      {
        entity.ParentId = mapping[parentNumberItem];
      }
    }

    await _unitOfWork.SaveChangesAsync();
  }
}

return RedirectToAction("Index"); // Перенаправление на другую страницу после загрузки

}

/// <summary> /// Метод для построения иерархического дерева из плоского списка /// </summary> private List<CostItemAddViewModel> BuildHierarchy(List<CostItemAddViewModel> items) { var lookup = items.ToLookup(i => GetParentNumberItem(i.NumberItem));

var rootItems = items.Where(i => string.IsNullOrEmpty(GetParentNumberItem(i.NumberItem))).ToList();

foreach (var item in rootItems)
{
  BuildChildren(item, lookup);
}

return rootItems;

}

/// <summary> /// Рекурсивный метод для добавления дочерних элементов /// </summary> private void BuildChildren(CostItemAddViewModel parent, ILookup<string, CostItemAddViewModel> lookup) { var children = lookup[parent.NumberItem]; foreach (var child in children) { parent.Children = parent.Children ?? new List<CostItemAddViewModel>(); parent.Children.Add(child); BuildChildren(child, lookup); } }

/// <summary> /// Получение родительского NumberItem /// </summary> private string GetParentNumberItem(string numberItem) { if (string.IsNullOrEmpty(numberItem)) return null;

var parts = numberItem.Split('.');
if (parts.Length <= 1)
  return null;

return string.Join(".", parts.Take(parts.Length - 1));

}

/// <summary> /// Метод для вычисления затрат с учетом иерархии /// </summary> private void CalculateCosts(List<CostItemAddViewModel> items) { foreach (var item in items) { if (item.Children != null && item.Children.Any()) { CalculateCosts(item.Children); item.MatEquipmCost = item.Children.Sum(c => c.MatEquipmCost ?? 0); item.SmpPnpCost = item.Children.Sum(c => c.SmpPnpCost ?? 0); item.TotalCost = item.Children.Sum(c => c.TotalCost ?? 0); } else { // Вычисление MatEquipmCost if (item.ConsumptionCoefficient.HasValue && item.TotalCount.HasValue && item.MatEquipmPrice.HasValue) { item.MatEquipmCost = item.ConsumptionCoefficient.Value * item.TotalCount.Value * item.MatEquipmPrice.Value; } else { item.MatEquipmCost = 0m; }

    // Вычисление SmpPnpCost
    if (item.TotalCount.HasValue)
    {
      item.SmpPnpCost = item.SmpPnpPrice * item.TotalCount.Value;
    }
    else
    {
      item.SmpPnpCost = 0m;
    }

    // Вычисление TotalCost
    item.TotalCost = (item.MatEquipmCost ?? 0) + (item.SmpPnpCost ?? 0);
  }
}

}

/// <summary> /// Метод для преобразования иерархического дерева обратно в плоский список /// </summary> private List<CostItemAddViewModel> FlattenHierarchy(List<CostItemAddViewModel> items) { var flatList = new List<CostItemAddViewModel>();

foreach (var item in items)
{
  flatList.Add(item);

  if (item.Children != null && item.Children.Any())
  {
    flatList.AddRange(FlattenHierarchy(item.Children.ToList()));
  }
}

return flatList;

}

public decimal CalculateSmpPnpCost(decimal? totalCount, decimal? smpPnpPrice) { if (!totalCount.HasValue || !smpPnpPrice.HasValue) { // Обработка отсутствующих значений, можно выбросить исключение или вернуть ноль return 0m; }

var smpPnp = totalCount.Value * smpPnpPrice.Value;
var roundedResult = Math.Round(smpPnp, 2, MidpointRounding.AwayFromZero);
return roundedResult;

}

public decimal CalculateMatEquipmCost(decimal? totalCount, decimal? matEquipmPrice) { // Проверяем наличие значений if (!totalCount.HasValue || !matEquipmPrice.HasValue) { // Обработка отсутствующих значений, можно выбросить исключение или вернуть ноль return 0m; }

var roundedTotalRound = Math.Round(totalCount.Value, 2, MidpointRounding.AwayFromZero);

var multipliedValue = roundedTotalRound * matEquipmPrice.Value;

var matEquipmCost = Math.Round(multipliedValue, 2, MidpointRounding.AwayFromZero);

return matEquipmCost;

}

private string DetermineType(string? nodeCode, string? unitOfMeasure) { if (string.IsNullOrWhiteSpace(nodeCode)&& !string.IsNullOrEmpty(unitOfMeasure)) { return "Материал"; }

return "Работа"; // По умолчанию считаем запись работой

}

private decimal? ParseNullableDecimal(string text) { return decimal.TryParse(text, out decimal result) ? result : (decimal?)null; }

private int? ParseNullableInt(string text) { return int.TryParse(text, out int result) ? result : (int?)null; }

Answer:

Ваш код выглядит довольно хорошо, но есть несколько моментов, на которые стоит обратить внимание. Я выделю некоторые из них:

Проверка на null для IFormFile: В методе Upload вы проверяете, что файл не равен null и его длина больше 0. Это хорошая практика, но вы также можете добавить проверку на file.ContentType, чтобы убедиться, что загружаемый файл имеет правильный тип (например, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet для Excel).
Обработка ошибок: В случае, если что-то пойдет не так при обработке файла (например, если файл поврежден или не соответствует ожидаемому формату), вы можете добавить обработку исключений, чтобы вернуть пользователю более информативное сообщение об ошибке.
Оптимизация маппинга: Если у вас есть много полей для маппинга, вы можете рассмотреть возможность использования AutoMapper для маппинга из CostItemAddViewModel в CostItem в одном вызове, вместо того чтобы делать это вручную.
Проверка на наличие дочерних элементов: В методе BuildChildren вы проверяете наличие дочерних элементов, но не проверяете, есть ли у родительского элемента уже добавленные дочерние элементы. Это может привести к дублированию дочерних элементов, если метод будет вызван несколько раз для одного и того же родителя.
Улучшение читаемости: Некоторые методы, такие как CalculateCosts, могут быть разбиты на более мелкие методы для улучшения читаемости и тестируемости. Например, вы можете создать отдельные методы для вычисления MatEquipmCost, SmpPnpCost и TotalCost.
Логирование: Рассмотрите возможность добавления логирования, чтобы отслеживать, что происходит в вашем приложении, особенно в методах, которые обрабатывают файлы и выполняют вычисления.
Проверка на наличие значений: В методах CalculateSmpPnpCost и CalculateMatEquipmCost вы проверяете наличие значений, но в случае отсутствия значений вы просто возвращаете 0. Возможно, стоит рассмотреть возможность выбрасывания исключения или логирования этой ситуации.
Использование async/await: Убедитесь, что все асинхронные операции правильно используют await, чтобы избежать блокировок.
Проверка на дублирование NumberItem: При добавлении новых элементов в базу данных стоит проверить, нет ли уже существующих записей с таким же NumberItem, чтобы избежать дублирования.
Код стиля: Убедитесь, что ваш код соответствует стандартам кодирования, принятым в вашей команде или проекте. Это может включать в себя использование единообразного стиля именования, отступов и комментариев.

В остальном, код выглядит достаточно хорошо структурированным и понятным. Если у вас есть конкретные вопросы или проблемы, с которыми вы столкнулись, пожалуйста, дайте

Code Review Suggestions for CostItemsController

Answered using OpenAI o1 mini by OpenAI o1