Как можно усовершенствовать мой примитивный код?

Question

[Yastr]

Всем привет. Имеются поверхностный знания jquery, но никогда не делал ничего "масштабного", сейчас возникла необходимость написать корзину, ринулся в бой)

Сделал, и вроде она кое как работает, но мне хотелось бы услышать Ваши комментарии (и нужно будет еще добавлять кнопки, а уже сложно работать с такой кучей переменных). Уверен, что большая часть кода просто бред) В общем если сможете отпишите как было бы лучше сделать.

Посмотреть можно здесь http://jsfiddle.net/zDGS4/
Вроде все прокомментировал.

И сюда еще на всякий случай скину js.

// +,- количество уникальной позиции
$(".plus").click(function() {
   var nc = parseInt($(this).parent("div").children('.count').text())+1;
   $(this).parent("div").children('.count').text(nc);
});
$(".minus").click(function() {
   var mc = parseInt($(this).parent("div").children('.count').text())-1;
   $(this).parent("div").children('.count').text(mc);
});

$(".add").click(function() {
   var th = $(this);
   var parCl = $("div[class^=item-]");
   // Получаем ид из класса
   var classNum = parseInt(th.parent(parCl).attr('class').replace(/\D+/g,""));
   // Помещяем все данные в соответствующие переменные
   var title = th.parent(parCl).children('.title').text();
   var price = parseInt(th.parent(parCl).children('.price').text());
   var count = parseInt(th.parent(parCl).children('.count').text());
   // Считаем первоначальную сумму
   var elemSumm = price*count;
   // Получаем сумму из инпута для последующего увеличения
   var mainSumm = parseInt(($("#summ").val()+0)/10);
   // Проверяем помещена ли позиция в корзину
   if($("div").is(".cart-item-"+classNum)){
      // Если да получаем колиество из уже существующего класса
      var newCount = parseInt($(".cart-item-"+classNum+" .cart-count").text());
      var countOut = count + newCount;
      $(".cart-item-"+classNum+" .cart-count").text(countOut)
      // Увеличиваем сумму
      var mainSummOut = mainSumm + elemSumm;
      $("#summ").val(parseInt(mainSummOut));
   }
   else{
      // Если нет добавляем
      $("#info").append("<div class='cart-item-"+classNum+"'><span class=\"cart-title\">"+title+"</span><span class=\"cart-price\">"+price+"</span><span class=\"cart-count\">"+count+"</span><div class=\"spoiler\"></div></div>");
      // Увеличиваем значение суммы
      var mainSummOut = mainSumm + elemSumm;
      $("#summ").val(parseInt(mainSummOut));
   }
   // Считаем и выводим количество уникальных позиций
   var blud = $("div[class^=cart-item-]").length;
   $("#blud").val(blud);
});

Answer 1

[Александр Кит]

из первой части:

function changeDelegate(diff) {
    
    return function(event) {
        var $count = $(this)
                .parent('div')
                .children('.count')
            ,
            mc = $count.text();
        $count.text(parseInt(mc, 10) + diff);
    };
}
$('.plus')
    .click(changeDelegate(1));
$('.minus')
    .click(changeDelegate(-1));

Comments

[Yastr]

подскажите пожалуйста как здесь задать минимум в 1 элемент?

[Александр Кит]

Вы какой минимум и какой элемент имеете в виду?

[Yastr]

.count, чтобы значение не опускалось ниже 1 едницы

[Yastr]

Пробовал своими силами, но все, что смог придумать не совсем работает)
return function(event) {
var $count = $(this)
.parent('div')
.children('.count') ,
mc = $count.text();
$count.text(parseInt(mc, 10) + diff);
if(parseInt(mc)<1){
$count.text(parseInt("1"));
}
};
}

Опускается до 0, потом возвращается на 1.

[Yastr]

понятно почему так происходит, но это все, что смог)

[Александр Кит]

///...
  mc = parseInt($count.text(), 10) + diff;
  if (mc < 1) mc = 1;
  $count.text(mc);

[Yastr]

Спасибо!

Answer 2

[aen]

Скрипт весь заверните в IIFE (function() {})();
Для parseInt() укажите систему счисления 10.
Я бы еще клики подчищал через event.stopPropagation().
Данные лучше хранить в data-* атрибутах.

Comments

[sim3x]

@aen еще бы указали, как правильно оборачивать get/set данных в data-*

Answer 3

[Вадим Яковлев]

$(".plus").click и прочие
заменить на
$(document).on('click',".plus",function(){});

$("div").is(".cart-item-"+classNum)
заменить на
$("div.cart-item-"+classNum).length!=0

это то, что сразу бросается в глаза

$(this).parent("div").children('.count')
заменить как вы сделали с th

Ну и вообще я бы на вашем месте сделал объект блока cart-item- (например, cartItem), включающий в себя html и свойства, включающие дочерние элементы
а дальше при событии click на .add делал var newCartItem = cartItem.clone()

Comments

[Alexey Svirschevskiy]

Что за вредные советы?

менять $(".plus").click $(document).on('click',".plus",function(){}); не нужно, зачем вешать событие на документ, если элемента может не быть на странице?

$("div").is(".cart-item-"+classNum)
тогда уж менять на
$(".cart-item-"+classNum).length

[Вадим Яковлев]

>>зачем вешать событие на документ, если элемента может не быть на странице?

Если элемента нет на странице - не надо подключать файл скрипта, биндящего событие.
А делать .bind('clickl,function(){}) или .click(function(){}) каждый раз, когда целевой элемент пропал или появился, имея счастливую возможность продублировать бинды - это, несомненно, полезный совет, так?

[Alexey Svirschevskiy]

Да, Вы правы, не заметил вставку html элемента.

[sim3x]

@FrimInc в зависимости от версии. В jQ1.8+ This method is a shortcut for .on( "click", handler ) api.jquery.com/click при ответах - делайте отсылку к документации

Answer 4

[enchikiben]

Много обращений к одному и тому же элементу, тут
th.parent(parCl)
$(".cart-item-"+classNum+" .cart-count")
$("#summ")
лучше выносить в переменную и избавляться от повторного обращение.

append лучше делать так .append( $('< div />',{"class":"cart-item-"+classNum}).... ) думаю суть ясна

Answer 5

[lega]

Вам нужно что-то более "высокоуровневое", например на Angular Light ваш пример работает "без js": jsfiddle.net/lega911/S6Pzx