Кто-нибудь хочет пересмотреть этот код JS для пользовательского скроллера?

Question

Я ни в коем случае не программист ... Я никогда не учился на компьютерных классах, не знаю много теории, но я все еще хакую код, который обычно работает. Я просто не знаю, насколько это ужасно. Я недавно написал этот (простой) кусочек JS и мне было интересно, что я могу получить отзывы об этом ...

Если это не подходящее место, дайте мне знать. Спасибо.

- Будет

    /*
    @author: Will Cavanagh
    @date: 2009-09-14
    * Custom scroll box
    */
    var customScroller = {
 intRegex : /^\d+$/,
 maxScroll : 0,
 inited : false,
 upColor : "#FFF",
 downColor : "#FFF", 
 defSpeed : "#FFF",

 //init function -- sets config values and initallizes jQuery slider.
 //@param options : object containing set up parameters
 //@return null
 init : function(options) {
  //if there are no options/colors specified, make empty object
  if(!options) { options = new Object(); };
  if(!options.scrollerColor) { options.scrollerColor = new Object(); };

  //assign variables, use defaults if not defined.
  var width = this.intRegex.test(options.width) ? options.width : 500
  var height = this.intRegex.test(options.height) ? options.height : 300;
  var vertical = options.vertical == null ? true : options.vertical;
  upColor = options.scrollerColor.upColor == null ? '#4a4a4a' : options.scrollerColor.upColor;
  downColor = options.scrollerColor.downColor == null ? '#333' : options.scrollerColor.downColor;
  var bkgdColor = options.scrollerColor.bkgdColor == null ? '#848484' : options.scrollerColor.bkgdColor;
  defSpeed = options.defaultSpeed == null ? '1000' : options.defaultSpeed;

  //set content width before measuring
  jQuery("#content-scroll").css({width: width});

  //get height of content, subtract height of pane to be shown in
  maxScroll = jQuery("#content-scroll").height() - height;
   //set the height of pane to hold content.  This is done after measuring content height for browser compatability reasons
  jQuery("#content-scroll").css({width: width, height: height});
  if(this.vertical) {
   var orientation = 'vertical';
  } else {
   var orientation = 'horizontal';
  }

  //create the JQuery.UI slider
  jQuery("#content-slider").slider({
   value: 100,
   orientation: 'vertical',
      animate: false,
     change: customScroller.handleSliderChange,
      slide: customScroller.handleSliderSlide,
     start: customScroller.handleSliderStart,
      stop: customScroller.handleSliderStop
     });

     jQuery(".ui-slider-handle").css({background:upColor});
     jQuery("#slider-bkg").css({background:bkgdColor});

     $("#content-scroll").mousewheel(function(objEvent, intDelta){
   scroll(intDelta * -30, 0);
  });

  inited = true;
 },

 //animates a scroll to the beginning of the content
 //@return null
 goTop : function() {
  if(!inited) { return };
  var scrollto = 0;
  jQuery("#content-scroll").animate({scrollTop: scrollto}, {queue:false, duration:defSpeed});
  jQuery("#content-slider").slider('option', 'value', 100*(1-(scrollto/maxScroll)));
 },


 //handler function bound to a slider change event.
 //@param e : event
 //@param ui : slider ui object
 //@return null
 handleSliderChange : function(e, ui) {
  if(!inited) { return };
  jQuery("#content-scroll").animate({scrollTop: ((100-ui.value) / 100) * (maxScroll) }, {queue:false, duration:defSpeed});
 },

 //handler function bound to a slider slide event.
 //@param e : event
 //@param ui : slider ui object
 //@return null
 handleSliderSlide : function(e, ui) {
  if(!inited) { return };
  jQuery("#content-scroll").attr({scrollTop: ((100-ui.value) / 100) * (maxScroll) });
 },

 //handler function bound to a slider start of slide event.
 //@return null
 handleSliderStart : function() {
  if(!inited) { return };
  jQuery(".ui-slider-handle").css({background:downColor});
 },

 //handler function bound to a slider end of slide event.
 //@return null
 handleSliderStop : function() {
  if(!inited) { return };
  jQuery(".ui-slider-handle").css({background:upColor});
 },


 //scroll by a given amount.
 //@param amt : amount to scroll by
 //@param speed : sroll animation speed, defaults to default speed defined in init params
 //@return null
 scroll : function(amt, speed) {
  if(!inited) { return };
  if(!speed) { speed = defSpeed; }
  var scrollto = jQuery("#content-scroll").scrollTop() + amt;
  if(scrollto > (maxScroll - 20)) scrollto = maxScroll; //near or past end of content, scroll to end
  if(scrollto < 20) scrollto = 0; //near or past beginning of content, scroll to beginning

  jQuery("#content-scroll").animate({scrollTop: scrollto}, {queue:false, duration:speed}); //animate content scroll
  jQuery("#content-slider").slider('option', 'value', 100*(1-(scrollto/maxScroll))); //update slider position
 }
    }

Answer 1

Несколько советов:

• new Object(); можно безопасно заменить на литерал объекта - { }.
• Вам не хватает некоторых кавычек после операторов (возможно, запустите его через JSLint).
• У вас есть необъявленные переменные, например, upColor и downColor. Объявите их с var.
• У вас есть только некоторые значения в "config". Почему остальные встроены?
• Лучше избегать «магических чисел», таких как 20 в вашем примере (scrollto < 20). Определите их отдельно под описательными именами.
• Некоторые из строк селектора - например, «# content-scroll» - повторяются на протяжении всего скрипта; вероятно, лучше всего перенести их в конфиг (делая вещи более расширяемыми и СУХИМЫМИ одновременно).
• Сравнение с null в этом случае кажется ненужным (options.scrollerColor.upColor == null). Я бы отбросил null и воспользовался бы преимуществом неявного преобразования типов (которое бы тоже ловило пустые строки!)

Answer 2

В дополнение к тому, что сказал kangax, это немного неуклюжий способ определения значений по умолчанию:

var width = this.intRegex.test(options.width) ? options.width : 500;
var height = this.intRegex.test(options.height) ? options.height : 300;
var vertical = options.vertical == null ? true : options.vertical;

Лучше всего использовать || Оператор:

var width = options.width || 500;
var height = options.height || 300;
var vertical = options.vertical || true;

Еще лучшим способом была бы функция, которая берет объект с параметрами по умолчанию и объединяет их с действительными предоставленными параметрами:

var defaultOptions = {width: 500, height: 300, vertical: true};
var options = applyDefaults(defaultOptions, options);

Кроме того ...

Вы можете рассмотреть возможность полного удаления этих if(!inited) { return } строк или замены их на if(!inited) { alert("not inited"); }, так как в противном случае вы маскируете ошибки. Когда кто-то пытается использовать ваш customScroller, но забывает запустить init(), то в настоящее время эта вещь просто не работает, даже не выдает ошибку, и будет довольно сложно выяснить причину, по которой все просто молча терпит неудачу. Лучше провалиться с огромным шумом.

И еще ...

Похоже, вам действительно нужно рассмотреть возможность принятия более объектно-ориентированного подхода. В настоящее время у вас есть все эти глобальные переменные inited, upColor, downColor и т. Д., Которые действительно хотят быть переменными экземпляра. Скажем, как-то так:

var CustomScroller = function(options) {
  this.options = options;
};
CustomScroller.prototype = {
  init: function() {
    ...
    this.inited = true;
  },
  goTop: function() {
    if (!this.inited) { alert("Not inited!"); }
    ...
  }
};

var myScroller = new CustomScroller({width: 100, height: 300, ...});
myScroller.init();
myScroller.goTop();

Answer 3

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

пример:

if(expression) {
    statement;
}

против

if(expression){ statement; }

Также рекомендуется всегда использовать скобки в выражениях блока, это предотвращает множество ошибок.

Пример

if(expression) {
  statement;
  statement;
}

против

if(expression) 
statement;
statement; //woops

У вас есть много конструкций типа «если нет», где if-do будет меньше строк и яснее.

пример

if(!expression) {
  return;
}
statement;

против

if(expression) {
  statement;
}