いったい何が変わったのか

以下のjavascriptは、ブレイクスルーjavascript34頁から49頁に記載されている頁上に存在する画像をモーダルウィンドウで拡大表示するソースです。
showメソッドとModalのプロトタイプオブジェクトである、createCounter()、next()、prev()に対して変更を加えております。
show()の変数をプロパティからローカル変数に変更したことで、show以外から接続できなくなり、書き換えられる心配がないことは、書籍に書いており、なるほどと思いましたが、
それ以外に、ソースを変更したことでどのような利点や変更した理由があるのでしょうか。

javascript
1function Modal(el) {
2	this.initialize(el);
3}
4
5Modal.prototype.initialize = function(el) {
6    this.$el = el;
7	this.$container = $("#modal");
8	this.$contents = $("#modal-contents");
9	this.$close = $("#modal-close");
10	this.$next = $("#modal-next");
11	this.$prev = $("#modal-prev");
12	this.$overlay = $("#modal-overlay");
13	this.$window = $(window);
14	this.handleEvents();
15};
16
17Modal.prototype.handleEvents = function() {
18	var self = this;
19
20	this.$el.on("click", function(e) {
21		self.show(e);
22		return false;
23	});	
24	
25	this.$close.on("click",function(e) {
26        self.hide(e);
27	    return false;	
28    });
29
30    this.$overlay.on("click",function(e) {
31	    self.hide(e);
32	    return false;
33    });
34	
35	this.$next.on("click", function(e) {
36		self.next(e);
37		return false;
38	});
39
40	this.$prev.on("click", function(e) {
41		self.prev(e);
42		return false;
43	});
44};
45
46Modal.prototype.show = function(e) {
47
48    var $target = $(e.currentTarget),
49	    src=$target.attr("href");
50
51	this.$contents.html("<img src=\"" + src +"\"/>");
52	this.$container.fadeIn();
53	this.$overlay.fadeIn();
54	//変更後
55	var index = $target.data("index");
56	this.countChange = this.createCounter(index,this.$el.length);
57	//変更後ここまで
58	return false;
59
60};
61
62Modal.prototype.hide = function(e) {
63	this.$container.fadeOut();
64	this.$overlay.fadeOut();
65};
66
67Modal.prototype.slide = function(index) {
68	this.$contents.find("img").fadeOut({
69		complete: function() {
70			var src = $("[data-index=\"" + index + "\"]").find("img").attr("src");
71			$(this).attr("src",src).fadeIn();
72		}
73	});
74};
75//変更後
76Modal.prototype.createCounter = function(index, len) {
77	return function(num) {
78	    return index = (index + num + len) % len;
79	};
80};
81
82Modal.prototype.next = function() {
83	this.slide(this.countChange( 1 ));
84};
85
86Modal.prototype.prev = function() {
87	this.slide(this.countChange( -1 ));
88};
89//変更後ここまで			
90var modal = new Modal($("#modal-thumb a"));
91//ブレイクスルーjavascript chapter2

変更前のソース

javascript
1//<中略>
2Modal.prototype.show = function(e) {
3
4    var $target = $(e.currentTarget),
5	    src=$target.attr("href");
6
7	this.$contents.html("<img src=\"" + src +"\"/>");
8	this.$container.fadeIn();
9	this.$overlay.fadeIn();
10	//変更前
11	this.index = $target.data("index");
12	//変更前ここまで
13	return false;
14	
15};
16//<中略>
17//変更前
18Modal.prototype.countChange = function(num,index,len) {
19    return (index+num+len) % len;
20};
21Modal.prototype.next = function() {
22    this.index = this.countChange(1,this.index,this.$el.length);
23	this.slide(this.index);
24};
25Modal.prototype.prev = function() {
26    this.index = this.countChange(-1,this.index,this.$el.length);
27	this.slide(this.index);
28};
29//変更前ここまで

html
1<!DOCTYPE html>
2<html lang="ja">
3<head>
4<meta charset="utf-8">
5<title>chapter2</title>
6<link rel="stylesheet" href="./index.css" />
7</head>
8<body>
9<div id="container">
10  <ul id="modal-thumb">
11    <li><a href="photo-01.jpg" data-index="0">
12    <img alt="" src="photo-01.jpg"></a></li>
13    <li><a href="photo-02.png" data-index="1">
14    <img alt="" src="photo-02.png"></a></li>
15    <li><a href="photo-03.png" data-index="2">
16    <img alt="" src="photo-03.png"></a></li>
17  </ul>
18</div>
19
20<div id="modal">
21  <div id="modal-container">
22   <!-- モーダルで表示する拡大画像を入れるための要素 -->
23    <div id="modal-contents"></div>
24    <!-- モーダルウィンドウの右上に表示するボタン -->
25    <div id="modal-close"><span class="glyphicon glyphicon-remove" aria-hidden="true"></span></div>
26    <div id="modal-prev"><span class="glyphicon glyphicon-chevron-left" aria-hidden="true"></span></div>
27    <div id="modal-next"><span class="glyphicon glyphicon-chevron-right" aria-hidden="true"></span></div>
28  </div>
29</div>
30
31<div id="modal-overlay"></div>
32<script src="http://code.jquery.com/jquery-1.11.1.min.js"></script>
33<script src="./index.js"></script>
34<script>
35</script>
36</body>
37</html>

css
1@charset "utf-8";
2
3ul, li {
4	padding: 0;
5	list-style: none;
6}
7#container {
8	margin-top: 20px;
9}
10#modal {
11	/* #modalと#modal-overlyは、サムネイルをクリックした時に表示されるものなので非表示にしている */
12	display: none;
13	position: absolute;
14	top: 0;
15	left: 0;
16	right: 0;
17	bottom: 0;
18	margin: auto;/*autoにすると自動的に適切な余白が設定される。widthを指定したうえで左右の余白をautoにすると、ボックスは水平方向の中央に揃う。*/
19	width: 750px;
20	height: 500px;/*500px*/
21	box-shadow: 0 4px 12px rgba(0,0,0,0.5);
22	border-radius: 6px;/*6px*/
23	overflow: hidden;
24	/* #modalは#modal-overlyよりも前面に表示させたいので、z-indexプロパティで重ね順を調整する。 */
25	z-index: 10;
26}
27#modal-container {
28	position: relative;/*要素が通常配置される位置を基準にして要素を配置する際に指定*/
29	background: #FFF;/*#FFF*/
30	width: 100%;
31	height: 100%;
32	overflow: hidden;
33}
34#modal-contents {
35	width: 100%;
36	height: 100%;
37}
38#modal-close, #modal-prev, #modal-next {
39	position: absolute;
40	top: 10px;
41	padding: 8px;
42	font-size: 8px;
43	line-height: 1;/*行の高さを指定する。単位を指定していないときは、フォントの大きさに指定した数値を乗算した数値が行の高さとなる。つまり8px*2だ。*/
44	border-radius: 50%;
45	background: #fff;
46	box-shadow: 0 0 1px rgba(0,0,0,0.4);/*長さ(正数だとボックス右側面と右下、負数だと左側面と左上))、ぼかし半径、広がり、*/
47	cursor: pointer;
48}
49#modal-close {
50	background: #ff6353;
51	right: 10px;
52}
53#modal-prev {
54	right: 80px;
55}
56#modal-next {
57	right: 50px;
58}
59
60#modal-overlay {
61	/* #modalと#modal-overlyは、サムネイルをクリックした時に表示されるものなので非表示にしている */
62	display: none;
63	position: absolute;
64	top: 0;
65	left: 0;
66	width: 100%;
67	height: 100%;
68	background: rgba(0,0,0,0.4);
69	cursor: pointer;
70}
71#modal-contents img {
72	max-width: 100%;
73}
74#modal-thumb {
75	text-align: center;
76}
77#modal-thumb li {
78	display: inline-block;
79	width: 160px;
80	margin-left: 5px;
81	margin-bottom: 5px;
82}
83#modal-thumb li img {
84	width: 100%;
85}
86

行動規範の内容に同意します

回答1件

ベストアンサー

コードのメンテナンス性が上がっていると思います。
たとえば、this.$el.lengthを別の値に変更したくなった時に、
変更前コードは2か所変更する必要があります。

javascript
1Modal.prototype.next = function() {
2    this.index = this.countChange(1,this.index,this.$el.length);  // ここ
3    this.slide(this.index);
4};
5Modal.prototype.prev = function() {
6    this.index = this.countChange(-1,this.index,this.$el.length);  // ここ
7    this.slide(this.index);
8};

変更後コードは1か所の変更で済みます。

javascript
1this.countChange = this.createCounter(index,this.$el.length);  // ここ

可読性とのトレードオフになることも多いのですが、同じコードを繰り返すことは避けた方がメンテナンス性が上がりますし、バグも減るかと思います。

蛇足ですが。
ここは私ならこう書きます。

javascript
1    var index = $target.data("index");
2    this.countChange = this.createCounter(index,this.$el.length);

↓

javascript
1    this.countChange = this.createCounter($target.data("index"),this.$el.length);

ローカル変数に持たせていますが特に他から参照をしているわけでもなさそうですし、そもそもクロージャにしているindexと参照も異なってしまっています。バグの原因になりそうな？

投稿2017/07/15 02:12

Lhankor_Mhy

総合スコア36074

aaaaaaaa

2017/07/18 10:45

ご回答ありがとうございました。 >>ローカル変数に持たせていますが特に他から参照をしているわけでもなさそうですし、そもそもクロージャにしているindexと参照も異なってしまっています。バグの原因になりそうな？重ね重ね申し訳ありませんが、読点より前は、つまり関数内関数からローカル変数indexを参照していない、ということでしょうか。間違っていたら申し訳ありません。読点よりあとの「そもそもクロージャにしているindexと参照も異なってしまっています。」というのは、$targetに設定された値とプロパティをdataメソッドで取得すると、何らかの理由で参照が異なってしまい、不具合の原因になるということでしょうか。

Lhankor_Mhy

2017/07/18 10:55

前者についてはおっしゃる通りです。このコードの中でこのindex変数をどこでも参照していないので、保持する必要はないだろう、ということです。後者については、なんといいますか、そのindex変数はいかにもスライダーの位置を保持していそうだけど、実はモーダルを開いた時のインデックス値を保持しているので、なんか間違えそうだなあ、ということです。変数名を変えるのもいいかもしれませんね。

行動規範の内容に同意します