【问题标题】:Anyone care to review this JS code for a custom scroller?有人愿意查看此 JS 代码以获取自定义滚动条吗?
【发布时间】:2010-11-28 06:15:57
【问题描述】:

我绝不是程序员...我从来没有真正参加过 Comp sci 课程,不太了解理论,但我仍然将通常有效的代码组合在一起。我只是不知道它实际上有多丑。我最近写了这个(简单的)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
 }
    }

【问题讨论】:

  • 你需要更新你的代码块——在当前状态下很难阅读
  • 从技术上讲,这不是代码审查的地方。就是说,我会好心的。杰森是对的;当我阅读它时,我并没有很好地解析这些块。使用更多空格并按回车键。 :-) 我还考虑尝试简化大量的三元条件。也许有一个“验证”例程。
  • 等等 - 这段代码是否按原样工作?还是您需要帮助解决其中的问题?
  • 如果您想更深入地了解良好的编码实践,Steve mcConnell 完成的代码是一个非常好的资源。见cc2e.com

标签: javascript jquery json jquery-ui


【解决方案1】:

小贴士:

  • new Object(); 可以安全地替换为对象文字 - { }。
  • 语句后缺少一些引号(可能通过 JSLint 运行)。
  • 您有未声明的变量,例如upColor 和 downColor。使用var 声明它们。
  • “config”中只有一些值。为什么其余的都是内联的?
  • 在您的示例 (scrollto &lt; 20) 中,最好避免使用诸如 20 之类的“幻数”。在描述性名称下分别定义它们。
  • 一些选择器字符串 - 例如“#content-scroll” - 在整个脚本中重复;最好将它们带入配置中(同时使事情更具可扩展性和 DRY)。
  • 在这种情况下(options.scrollerColor.upColor == null)似乎没有必要与null 进行比较。我会放弃 null 并改用隐式类型转换(这也会捕获空字符串!)

【讨论】:

  • 你能详细说明你的最后一点吗? (“改为利用隐式类型转换......”)听起来很有趣。
  • Dan,而不是if (foo == null) { ... },我只使用if (foo) { ... }(并且让if 进行隐式类型转换)。只有当foo 是null 或undefined(如果是===,那么它只是null)时,前一个表达式才会计算块。然而,后者会“捕捉”任何所谓的 真实值 - 任何非原始(即对象,包括函数)、非 0 数字、非空字符串、true 布尔值等。
  • 谢谢——非常有用的反馈。使用变量来保存这些“幻数”而不管其功能如何,通常被认为是一种好的做法吗?
  • 威尔,这些神奇的数字往往会随着时间而变得混乱。这几乎是命名它们的主要理由。这些变量通常是常量,但不一定(JS当然没有常量;有些人使用大写约定)。
【解决方案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();

【讨论】:

  • 谢谢。一个问题,“var vertical = options.vertical || true;”不会总是结果为真吗?
  • 是的……没错,||在这种情况下,运算符并不是一个很好的选择。这就是为什么我还建议,更好的方法是使用某种 applyDefaults() 函数。
【解决方案3】:

主要是,不要害怕空格。将块中的语句与表达式放在不同的行上可以更容易地查看表达式的结束位置和语句的开始位置。

示例:

if(expression) {
    statement;
}

对比

if(expression){ statement; }

另外,在块语句中始终使用括号也是一个好习惯,这样可以防止很多错误。

例子

if(expression) {
  statement;
  statement;
}

对比

if(expression) 
statement;
statement; //woops

你有很多 if-not-return 结构,其中 if-do 会更少行更清晰。

例子

if(!expression) {
  return;
}
statement;

对比

if(expression) {
  statement;
}

【讨论】:

  • 感谢您的反馈——听起来我的 if(!inited) 构造并不受欢迎......很高兴知道。
猜你喜欢
  • 1970-01-01
  • 1970-01-01
  • 1970-01-01
  • 1970-01-01
  • 1970-01-01
  • 1970-01-01
  • 2022-07-19
  • 2021-10-13
  • 1970-01-01
相关资源
最近更新 更多