Есть ли ляпы в коде?

Question

[Vit]

Добрый день. Написал первый плагин jQuery для работы с таблицами.

pastebin.com/iigURNnM



Посмотрите пожалуйста, есть ли ляпы в коде, ошибки очевидные? М.б. можно улучшить код? Спасибо.

Answer 1

[Ivan Bogachev]

Комментарии в коде бесполезные, только глаза мозолят:

index: function (settings) {
    // Метод контроллера index
....
create: function (settings) {
    // Метод контроллера create
....

Если уж вы описываете свой код - делайте это с умом, посмотрите какие есть средства для генерации документации, например JSDoc

Используйте фигурные скобки и отступы везде, где только можно. Я, разумеется, понимаю, что "стильно модно молодежно" писать if в одну строку, но такого рода конструкции взрывают мозг:

....
else data = settings;
if (typeof data !== "string") data = $.param(data);
if (method == 'post') return $.post(url, data + '&_method=' + method_hidden);
else return $.get(url, data);
....

И, если еще придираться, - пустые строки после объявления переменных, после if/else, и.т.д. улучшают читабельность.

Не используйте ключевые слова из es6 где попало:
class: '.jsgrid-container',

Есть некоторая непоследовательность - иногда вы выносите объявления всех переменных в начало функции, иногда нет. Имеет смысл определиться и использовать что-то одно.

Да, и киньте ссылку на codepen что-ли, а то в 500 строк кода ни разу не понятно работает ли там что-то (и что оно вообще делает). И к этому хочется добавить - посмотрите в сторону систем сборки (Grunt/Gulp/...)на ваш вкус - скорее всего эти 500 строк можно разбить на части поменьше, станет проще ориентироваться в происходящем.

Comments

[Vit]

Большое спасибо. Поработаю над кодом.
Плагин нужен для CRUD манипуляций с данными из БД. Плюс поиск, фильтры, постраничный вывод, дополнительные кнопки.

Как внешне выглядит:

Есть некоторая непоследовательность - иногда вы выносите объявления всех переменных в начало функции, иногда нет. Имеет смысл определиться и использовать что-то одно.

Да, верно. Объявления переменных иногда делаю перед кодом, который их использует на тот случай, если понадобится вынести этот код в функцию.

Answer 2

[Виталий Инчин ☢]

Одна логическая ошибка в коде точно есть. Объясню ее на примере.

function setHandler(el){

   var obj = new MyController(el);

  $(el).on("click", function hand(){
      alert(obj.name);
  });

}

setHandler(element1);
setHandler(element2);

После выполнения данного кода в память браузера попадают:

• Функция setHandler
  
• Два объекта типа MyController
  
• Две функции hand - обработчики события onclick.
  

Да-да, для каждого вызова setHandler создается своя функция hand. Две (три, десять или даже миллион) разные функции с одинаковым телом. Все, что их отличает - область видимости, в которой они объявлены (к примеру, внутри каждой из них доступен свой obj).

Но, спрашивается, как мы можем оптимизировать потребление памяти в данной ситуации? А вот так:

function hand(){

  var obj = $(this).data("obj");

  alert(obj.name);

}

function setHandler(el){

   var obj = new MyController(el);

  $(el)
    .data("obj", obj) //Как вариант
    .on("click", hand)
  ;

}

setHandler(element1);
setHandler(element2);

Вынесем hand в более высокую область видимости.

Теперь в памяти сохранены:

• Функция setHandler
  
• Функция hand
  
• Два объекта MyController
  

Comments

[Aves]

Я совсем не гуру, и может быть ошибаюсь, но современные движки этот момент спокойно оптимизируют. Нагуглил https://github.com/petkaantonov/bluebird/wiki/Opti... и набросал анологичный для данного случая код:

function printStatus(fn) {
  switch (%GetOptimizationStatus(fn)) {
    case 1: console.log("Function is optimized"); break;
    case 2: console.log("Function is not optimized"); break;
    case 3: console.log("Function is always optimized"); break;
    case 4: console.log("Function is never optimized"); break;
    case 6: console.log("Function is maybe deoptimized"); break;
    case 7: console.log("Function is optimized by TurboFan"); break;
    default: console.log("Unknown optimization status"); break;
  }
}

var EventEmitter = require('events').EventEmitter;
var e = new EventEmitter();

function My(name) {
  this.name = name;
}

function test(name) {
  var obj = new My(name);
  e.once('test', function cbInTest(self) {
    if (self) return;
    console.log(obj.name);
    cbInTest(true);
    cbInTest(true);
    %OptimizeFunctionOnNextCall(cbInTest);
    cbInTest(true);
    printStatus(cbInTest);
  });
}

test(1);
test(2);
test(3);

e.emit('test');

Запустив

node --trace_opt --trace_deopt --allow-natives-syntax test.js

Предполагаю, что строка

[optimizing 0x21c8ad010a59 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)> - took 0.040, 0.242, 0.094 ms]

говорит о том, что движок создаёт одну общую функцию, а затем использует её повторно:

[found optimized code for 0x21c8ad09f081 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)>]
[found optimized code for 0x21c8ad09f0c9 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)>]

[Aves]

вывод в консоль

1
[compiling method 0x21c8ad010a59 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)> using Crankshaft]
[optimizing 0x21c8ad010a59 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)> - took 0.040, 0.242, 0.094 ms]
Function is optimized
2
[found optimized code for 0x21c8ad09f081 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)>]
Function is optimized
3
[found optimized code for 0x21c8ad09f0c9 <JS Function cbInTest (SharedFunctionInfo 0xa46935601e1)>]
Function is optimized

[Виталий Инчин ☢]

Aves: довольно интересно, никогда не проводил подобные проверки и не могу уверенно ответить.
Тем не менее, логично предположить, что лексическое окружение (LexicalEnvironment) для каждой функции будет сохранятся свое. А вот тело функции, движок, оптимизирует и сохраняет единожды.

[Николай]

Ситуация аналогична с ситуацией, когда внутри конструктора мы пишем this.foo = function() {}; v8 умеет с этим справляться и создает общую функцию для всех экземпляров. (Что кстати не значит, что стоит так делать).

[Виталий Инчин ☢]

Николай, Aves: вообще, речь идет о логике самого кода, не о логике движков.
Нужно прежде всего мысленно "компилировать" этот код у себя в голове, согласно правилам и особенностям, которые нам указывают js-стандарты и смотреть что вышло. А оптимизировать код под определенные движки и браузеры - уже следующим шагом.

P.s: Но мне стало интересно - есть ли информация на обсуждаемую нами тему по другим движкам?

[Aves]

Виталий Инчин ☢: Все современные распространённые движки являются не интерпретаторами javascript, а JIT-компиляторами, строят абстрактное синтаксическое дерево, анализируют и оптимизируют его.

Мой основной посыл в том, что вещи, которые в теории по спецификации выглядят однозначно, на практике имеют нюансы, и погоня за преждевременной оптимизацией может привести даже к её потере.

В данном случае функция hand в реальности, скорее всего, скомпилирована в разделяемую не будет, для этого, по идее, нужно вызвать её несколько десятков раз. Но и потери памяти большой не будет - лексическое окружение тоже оптимизируется, причем вроде сразу, и будет содержать только obj.

В итоге в начальном варианте на каждый вызов setHandler в памяти будут создаваться hand и obj, при клике hand будет происходить обращение к объекту в замыкании. В вашем оптимизированном варианте при вызове setHandler в памяти будет создаваться только obj, а вот при клике будет вызов $(this) - медленное обращение к DOM и создание объекта jQuery. Выигрыш по памяти если и будет, то очень маленьким, а проигрыш в производительности - будет точно, и довольно заментым.

Лично я бы предпочёл начальный вариант.

[Виталий Инчин ☢]

Aves: я использовал встроеное jQuery-хранилище ($.fn.data) как пример, с рассчетом на то, что устанавливая обработчик на элемент, мы планируем работать с jQuery & DOM.

>Мой основной посыл в том
Я вас услышал. В общем, всего нужно в меру :)

>вызов $(this) - медленное обращение к DOM и создание объекта jQuery
Вы шутите? Медленное?