Как можно улучшить код к лучшему?

11 октября 2019, 20:20

улучшить - читабельность, модульность, производительность. В фронте не силен. Условие: чистый js либо jQuery. Единственная модульность - у каждой вьюшки свой js (MVC на сервере). Пример кода js одной из view:

jQuery(document).ready(function () {
    let ExpenseContents = [];
    let sum = 0;
    let CreatedDate = new Date();
    // Клик на добавить содержимое расхода
    jQuery("#btnAddingExpenseToTable").click(function () {
        let num = 0;
        ExpenseContents.push({
            code: $(this).parent().parent().find("#inputExpenseCode").val(),
            product: { Id: $(this).parent().parent().find("#selected_product").val(), Title: $(this).parent().parent().find("#selected_product option")[$(this).parent().parent().find("#selected_product")[0].selectedIndex].innerHTML },
            count: $(this).parent().parent().find("#inputExpenseCount").val(),
            price: $(this).parent().parent().find("#inputExpensePrice").val(),
            sum: $(this).parent().parent().find("#inputExpenseCount").val() * $(this).parent().parent().find("#inputExpensePrice").val()
        });
        let index = ExpenseContents.length - 1;
        let currentExpense = ExpenseContents[index];
        jQuery("#tableExpenses").append("<div class='row my-2'><div class='col idExpense'>" +index +"</div><div class='col'>" + currentExpense.code + "</div><div class= 'col'>" + currentExpense.product.Title + "</div><div class='col'>" + currentExpense.count + "</div><div class='col'>" + currentExpense.price + "</div><div class='col'>" + currentExpense.sum + "</div><div class='col text-center'><button class='btn btn-block btn-outline-warning' id='btnUpdatingExpenseInTable'>Изменить</button></div><div class='col text-center'><button class='btn btn-block btn-outline-danger' id='btnDeleteExpenseInTable'>Удалить</button></div></div>");
        ExpenseContents.forEach(function (el) {
            sum += el.sum;
        });
        console.log(sum);
        jQuery("#amountExpense").text(sum.toString());
    });
    // Клик по кнопке "провести"
    jQuery("#btnExpenseHold").click(function () {
        requestToExpense(new Date, CreatedDate, jQuery("#selected_client").val(), sum, 1, new Date(), ExpenseContents);
    });
    // Клик по кнопке "отложить"
    jQuery("#btnExpenseDelayed").click(function () {
        requestToExpense(new Date, CreatedDate, jQuery("#selected_client").val(), sum, 0, null, ExpenseContents);
    });
    // Клик по кнопке "отменить"
    jQuery("#btnExpenseCanseled").click(function () {
        requestToExpense(new Date, CreatedDate, jQuery("#selected_client").val(), sum, 2, null, ExpenseContents);
    });
    // Клик по кнопке "очистить"
    jQuery("#btnClearEditCol").click(function () {
        $(this).parent().parent().find("input").each(function () {
           $(this).val("");
        });
    });
    // Клик по кнопке "изменить"
    jQuery("#btnUpdatingExpenseInTable").click(function () {
        $(this).parent().parent().parent().find(".update-row").each(function () {
            $(this).css("display", "block");
        }).end().parent().parent().parent().find(".view-row").each(function () {
            $(this).css("display", "none");
        });
    });
    // Клик по кнопке "сохранить"
    jQuery("#btnSaveUpdatingExpenseInTable").click(function () {
        console.log($(this).parent().parent().parent().find(".idExpense").text()); //id!!!
        // изменить id^ в массиве >>>>> ExpenseContents по $(this)
    });
});
/// Вносим в БД расход и его содержимое
function requestToExpense(date, dateCreate, clientId, sum, status, dateSpeading,mass) {
    let data = JSON.stringify({ Date: date, ClientId: clientId, Sum: sum, ExpenseCreated: dateCreate, ExpenseStatus: status, TimeSpeadingExpense: dateSpeading });
    jQuery.ajax({
        type: "POST",
        crossDomain: true,
        url: "https://localhost:44334/Data/Expense/Create",
        contentType: "application/json; charset=utf-8",
        data: data,
        processData: false,
        success: function (res) {
            let contents = [];
            mass.forEach(function (el) {
                contents.push({ Code: el.code, ProductId: el.product.Id, Count: el.count, Price: el.price, Sum: el.sum, ExpenseId: res });
            });
            let data2 = JSON.stringify({ contents });
            jQuery.ajax({
                type: "POST",
                crossDomain: true,
                url: "https://localhost:44334/Data/Contents/Create",
                contentType: "application/json; charset=utf-8",
                data: data2,
                processData: false,
                success: function (res) {
                },
                Error: function () {
                }
            });
        },
        Error: function () {
        }
    });
}

Приму любые предложения по улучшению данного кода, спасибо :)

Answer 1

Можно улучшить:

Избавиться от конструкций вида $(this).parent().parent(). Если поменяется верстка, то будет тяжело это менять, плюс потенциальные баги.
Заменить обработчики кнопок вида jQuery("#btnExpenseHold").click(function () {... на единый. Необходимые данные хранить в data-* атрибутах.
Вызов двух АПИ в requestToExpense слить один. На самом деле спорный момент. С одной стороны нарушаем принцип единой ответственности. С другой стороны, если у вас выполнится Expense/Create но не выполнится Contents/Create данные могут быть в неконсистентном состоянии.
Создать свои методы для POST, PUT, GET, DELETE, что бы можно было обрабатывать в ошибки в одном месте, хранить url до АПИ тоже в одном месте.
Придерживаться единого стиля в названиях переменных. Стандартом де-факто для JS является camelCase.
Также везде использовать что-то одно. Или $ или JQuery.
Не писать конструкции вида $('.smth').append('<div>'+someText+'</div>'). Чревато ошибками, если в someText будут спецсимволы.

Код с учетом всех изменений

$(document).ready(function() { 
  let ExpenseContents = []; 
  let sum = 0; 
  let CreatedDate = new Date(); 
  // Клик на добавить содержимое расхода 
  $("#btnAddingExpenseToTable").click(function() { 
    let num = 0; 
    const $form = $('#create-form'); 
    const formVal = (selector) => { 
      return $form.find(selector).val() 
    }; 
    ExpenseContents.push({ 
      code: formVal("#inputExpenseCode"), 
      product: { 
        Id: formVal("#selected_product"), 
        Title: $form.find("#selected_product option")[$form.find("#selected_product")[0].selectedIndex].innerHTML 
      }, 
      count: formVal("#inputExpenseCount"), 
      price: formVal("#inputExpensePrice"), 
      sum: formVal("#inputExpenseCount") * formVal("#inputExpensePrice") 
    }); 
    let index = ExpenseContents.length - 1; 
    let currentExpense = ExpenseContents[index]; 
 
    const row = $('#example_row').clone(); 
    const setRowText = (selector, value) => { 
      row.find(selector).text(value); 
    }; 
    setRowText('.idExpense', index); 
    setRowText('.code', currentExpense.code); 
    setRowText('.title', currentExpense.product.Title); 
    setRowText('.count', currentExpense.count); 
    setRowText('.price', currentExpense.price); 
    setRowText('.sum', currentExpense.sum); 
 
    $("#tableExpenses").append(row); 
 
    ExpenseContents.forEach(function(el) { 
      sum += el.sum; 
    }); 
    console.log(sum); 
    $("#amountExpense").text(sum.toString()); 
  }); 
 
  // Клик по кнопкам 
  $(".btn-action").click(function() { 
    const $button = $(this); 
    requestToExpense(new Date, CreatedDate, $("#selected_client").val(), sum, $(this).data('status'), new Date($(this).data('time')) || null, ExpenseContents); 
  }); 
 
  // Клик по кнопке "изменить" 
  $("#btnUpdatingExpenseInTable").click(function() { 
    $('.row-id').find(".update-row").each(function() { 
      $(this).css("display", "block"); 
    }) 
    $('.another-id').find(".view-row").each(function() { 
      $(this).css("display", "none"); 
    }); 
  }); 
  // Клик по кнопке "сохранить" 
  $("#btnSaveUpdatingExpenseInTable").click(function() { 
    console.log($(this).parent().parent().parent().find(".idExpense").text()); //id!!! 
    // изменить id^ в массиве >>>>> ExpenseContents по $(this) 
  }); 
 
 
  /// Вносим в БД расход и его содержимое 
  function requestToExpense(date, dateCreate, clientId, sum, status, dateSpeading, mass) { 
    let data = { 
      Date: date, 
      ClientId: clientId, 
      Sum: sum, 
      ExpenseCreated: dateCreate, 
      ExpenseStatus: status, 
      TimeSpeadingExpense: dateSpeading 
    }; 
    data.contents = []; 
    mass.forEach(function(el) { 
      data.contents.push({ 
        Code: el.code, 
        ProductId: el.product.Id, 
        Count: el.count, 
        Price: el.price, 
        Sum: el.sum 
      }); 
    }); 
    POST('ExpenseContents/Create', data, (res) => {}); 
  } 
 
}); 
 
// This variable should be defined in a separate file. 
const API = "https://localhost:44334/Data/"; 
 
// This function should be defined in a separate file. 
function POST(url, data, success, error) { 
  return $.ajax({ 
    type: "POST", 
    crossDomain: true, 
    url: API + url, 
    contentType: "application/json; charset=utf-8", 
    data: data, 
    success: success, 
    error: function() { 
      console.log('this is standard error handler'); 
      error && error(); 
    } 
  }); 
}

.hidden { 
  display: none; 
}

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script> 
 
<div id="example_row" class='hidden row my-2'> 
  <div class='col idExpense'></div> 
  <div class='col code'></div> 
  <div class='col title'></div> 
  <div class='col count'></div> 
  <div class='col price'></div> 
  <div class='col' sum></div> 
  <div class='col text-center'><button class='btn btn-block btn-outline-warning' id='btnUpdatingExpenseInTable'>Изменить</button></div> 
  <div class='col text-center'><button class='btn btn-block btn-outline-danger' id='btnDeleteExpenseInTable'>Удалить</button></div> 
</div>