柳屋

ソースコードのリファクタからよもやままで

Javascriptをリファクタしてみました2

今回は入力値を時間 + 分表記に変更するjavascriptです コードのボリュームがあったので少し長くなりました。

javascript

var SAMPLE = {};

SAMPLE.ConvertInputTime = function(trigger) {
    this.$trigger = trigger;
    this.init();
};
SAMPLE.ConvertInputTime.prototype = {
    IS_ERROR: 'err',
    init: function() {
        this.$parents = $('.jscConvertTime');
        this.$output = this.$parents.find('.jscConvertTimeOutput');
        this.bindEvent();
    },
    bindEvent: function() {
        var self = this;
        this.$trigger.on('input blur', function() {
            self.convert($(this));
        });
    },
    convert: function($trigger) {
        var maxlength = this.$trigger.attr('maxlength'),
            inputValue = $trigger.val(),
            inputValue = inputValue.replace(/[A-Za-z0-9]/g, function($trigger) {
                return String.fromCharCode($trigger.charCodeAt(0) - 65248)
            }).slice(0, maxlength),
            alertText = this.getAlertText(inputValue);

        this.$trigger.val(inputValue);
        this.changeBgColor(inputValue);
        this.$output.text(alertText);
    },
    getAlertText: function(inputValue) {
        var time = Math.floor(inputValue / 60);
        var minutes = inputValue % 60;

        if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
            return '5分単位で入力してください';
        } else if (time === 0 && minutes < 60) {
            return minutes + '分';
        } else if (minutes === 0) {
            return time + '時間';
        } else {
            return time + '時間' + minutes + '分';
        }
    },
    changeBgColor: function(inputValue) {
        if (inputValue % 5 !== 0) {
            this.$parents.addClass(this.IS_ERROR);
            return;
        }
        this.$parents.removeClass(this.IS_ERROR);
    },
};

$(function() {
    new SAMPLE.ConvertInputTime($('.jscConvertTimeInput'));
});

HTML

<div style="width: 300px; margin: 20px 0 0 30px;" class="jscConvertTime">
  <input type="number" name="rsvTermTime" maxlength="3" class="jscConvertTimeInput"><span class="dib vaM"></span>
<div class="mt3 jscConvertTimeOutput">5分単位で入力してください</div>
</div>

変数名

変数名って処理には直接関係ないんですが、メンテナンス性考えると適切な名前が良いですよね。 あとよくあるのが単数形、複数形が間違ってるのがあります。

まずは以下の定数 IS_ERROR

SAMPLE.ConvertInputTime.prototype = {
    IS_ERROR: 'err',
    init: function() {

IS〜から始まるものってboolean型であることが 暗黙知としていることが多いです。 なので混乱を避けるためにIS_ERRORではなく CLASS_ERROR等の方が適切です

次に以下の変数の$parents

        this.$parents = $('.jscConvertTime');
        this.$output = this.$parents.find('.jscConvertTimeOutput');
        this.bindEvent();```

親になっているので複数系にはなりません。 なのでここは単数系の $parentが正しいです

次に以下の変数のtime

    getAlertText: function(inputValue) {
        var time = Math.floor(inputValue / 60);
        var minutes = inputValue % 60;

        if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
            return '5分単位で入力してください';
        } else if (time === 0 && minutes < 60) {
            return minutes + '分';
        } else if (minutes === 0) {
            return time + '時間';
        } else {
            return time + '時間' + minutes + '分';
        }
    },

minitesがあるならそれに対比する hourになるのでは?!

プロトタイプの呼び出し方法

今回はプロトタイプで作ってるんですが渡すクラスが少々気になります。 引数がjscConvertTimeInputなんですが、ラッパー要素になっていない。 なのでfindとclosest、parentが混在して統一性が悪いです なので基本的には処理全体のラッパー要素を渡しましょう

まずはこうなります

var SAMPLE = {};

- SAMPLE.ConvertInputTime = function(trigger) {
+ SAMPLE.ConvertInputTime = function($base) {
-    this.$trigger = trigger;
+    this.$base = $base;
    this.init();
};
SAMPLE.ConvertInputTime.prototype = {
    IS_ERROR: 'err',
    init: function() {
-        this.$parents = $('.jscConvertTime');
-        this.$output = this.$parents.find('.jscConvertTimeOutput');
+       this.$output = this.$base.find('.jscConvertTimeOutput');
+       this.$trigger = this.$base.find('.jscConvertTimeInput');

        this.bindEvent();
    },

〜省略〜

$(function() {
-    new SAMPLE.ConvertInputTime($('.jscConvertTimeInput'));
+    new SAMPLE.ConvertInputTime($('. jscConvertTime'));
});

不要な引数

イベントを付与している以下の箇所

 bindEvent: function() {
        var self = this;
        this.$trigger.on('input blur', function() {
            self.convert($(this));
        });
    },

$(this) を渡してます。これってちょくちょく見るんですが 渡す必要はないのでは?

eachで回しててそれぞれに対して個別にイベントを付けるならわかるんですが。 なので以下の通りとなります

〜省略〜
    bindEvent: function() {
        var self = this;
        this.$trigger.on('input blur', function() {
-            self.convert($(this));
+           self.convert();
        });
    },
-    convert: function($trigger) {
+    convert: function() {
        var maxlength = this.$trigger.attr('maxlength'),
-            inputValue = $trigger.val(),
+           inputValue = this.$trigger.val(),
-            inputValue = inputValue.replace(/[A-Za-z0-9]/g, function($trigger) {
+            inputValue = inputValue.replace(/[A-Za-z0-9]/g, function(this.$trigger) {
                return String.fromCharCode($trigger.charCodeAt(0) - 65248)
            }).slice(0, maxlength),
            alertText = this.getAlertText(inputValue);

        this.$trigger.val(inputValue);
        this.changeBgColor(inputValue);
        this.$output.text(alertText);
 〜省略〜

if文のブロックについて

elseやelse ifを多用するのがよく見受けれます。

もちろんケースバイケースにはなるんですが、あまりelseやelse ifを 使いすぎるのは良くないなと思います

elseやelse ifを使うと処理のネストが深くなったり、else ifが無駄に増えたりする傾向になりがちです

それにif文は条件判定単位でひとまとめにするのが処理としてもわかりやすく見通しがよくなります

getAlertText: function(inputValue) {
        var time = Math.floor(inputValue / 60);
        var minutes = inputValue % 60;
        
        if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
            return '5分単位で入力してください';
        } else if (time === 0 && minutes < 60) {
            return minutes + '分';
        } else if (minutes === 0) {
            return time + '時間';
        } else {
            return time + '時間' + minutes + '分';
        }
    },

上記ブロックのif(inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') は例外処理ですよね。 例外処理とメイン処理を同じif文にするのはソースの文脈としても違和感があります。 例外がまた追加することになればどうする?と考えれば分けるべきです。 例外は例外として早期リターンで処理を終わらす。 そうするとこうなります

getAlertText: function(inputValue) {
        var time = Math.floor(inputValue / 60);
        var minutes = inputValue % 60;
        
        if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
            return '5分単位で入力してください';
-        } else if (time === 0 && minutes < 60) {
+        }

+       if (time === 0 && minutes < 60) {
            return minutes + '分';
        } else if (minutes === 0) {
            return time + '時間';
        } else {
            return time + '時間' + minutes + '分';
        }
    },

こうすることによって例外処理とメイン処理が分割されコードとして見通しが良くなりました。

メイン処理とif文

先ほどのリファクタで例外とメイン処理が分離され見通しが良くなったのですが、メイン処理がif文の中に書かれているのはきな臭い感じがします。

if文の中にメイン処理を書いてしまうとネストが深くなる傾向にあります

それに分だけ返す、時間だけ返すのが処理として等価でしょうか?

分割した方がスッキリしないですかね?

さらにそれぞれがreturnで終わらせてるのも違和感があります。

ここは以下のように書いた方が拡張性も高くなります

getAlertText: function(inputValue) {
    var time = Math.floor(inputValue / 60);
    var minutes = inputValue % 60;
    
    if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
        return '5分単位で入力してください';
     }

-   if (time === 0 && minutes < 60) {
-        return minutes + '分';
-    } else if (minutes === 0) {
-        return time + '時間';
-    } else {
-        return time + '時間' + minutes + '分';
-    }

+   if(minute) {
+       minute = minute + '分';
+   }
+   if(hour) {
+       hour + '時間';
+   }
+   return hour + minute
},

引数に数式を

引数に数式を渡すのを初めてみた時は目から鱗感が半端なかったです

hoge(a > b)

みたいなやつね
結構拒否反応示す人も多いんですが、これはこれで行数や変数定義の節約にもなるし俺はちょこちょこ使います。
使い過ぎると確かに見難いコードにはなったりするので注意は必要ですが

convert: function($trigger) {
〜省略〜
    this.$trigger.val(inputValue);
    this.changeBgColor(inputValue);
    this.$output.text(alertText);
〜省略〜
},
〜省略〜
changeBgColor: function(inputValue) {
    if (inputValue % 5 !== 0) {
        this.$parents.addClass(this.IS_ERROR);
        return;
    }
    this.$parents.removeClass(this.IS_ERROR);
},

上記ソースの if (inputValue % 5 !== 0) { 判定するためにinputValueって引数があるんですが この変数って結局上記判定式でしか使ってない。

そのためだけの変数であるならば、変数を渡す意味ってどうなんでしょう? 判定式だけ渡せばもう少し簡潔に書けます

convert: function($trigger) {
〜省略〜
    this.$trigger.val(inputValue);
-    this.changeBgColor(inputValue);
+   this.changeBgColor(inputValue % 5 !== 0);
    this.$output.text(alertText);
〜省略〜
},
〜省略〜
- changeBgColor: function(inputValue) {
+ changeBgColor: function(result) {// 変数名がそのままだと気持ち悪いのでリネーム

-    if (inputValue % 5 !== 0) {
-        this.$parents.addClass(this.IS_ERROR);
-        return;
-    }
-    this.$parents.removeClass(this.IS_ERROR);

// inputValue % 5 !== 0は結局true or falseなので、結果としてやってることは同じ
+   if(result) {
+       this.$parents.addClass(this.IS_ERROR);
+   }else{
+       this.$parents.removeClass(this.IS_ERROR);
+   }
},

toggleClass

jQueryにtoggleClassってあります。 これの第2引数にtrue or falseを指定してやれば、クラスの付け替えが制御できます trueならクラスをつける falseならクラスを外す

これを利用すれば上記ソースが一行ですみます

-   if(result) {
-       this.$parents.addClass(this.IS_ERROR);
-   }else{
-       this.$parents.removeClass(this.IS_ERROR);
-   }

+     this.$parents.toggleClass(this.IS_ERROR, result);
},

上記ソースの話とは違うんですが、 toggleClass自体便利だとは思うんですが、制御できへんのが怖いです クラスが付いてるかどうかで処理が変わるってことは、処理の責務の分離ができていないと思うんです。html側に依存されてる。 なので、使うとしたら第二引数を指定してロジック側がきちんと処理するのが思わぬバグも出さないのかなと

完成形

長くなりましたが最終的にはこうなりました

var SAMPLE = {};

SAMPLE.ConvertInputTime = function($base) {
    this.$base = $base;
    this.init();
};
SAMPLE.ConvertInputTime.prototype = {
    CLASS_ERROR: 'err',
    init: function() {
        this.$output = this.$base.find('.jscConvertTimeOutput');
        this.$trigger = this.$base.find('.jscConvertTimeInput');

        this.bindEvent();
    },
    bindEvent: function() {
        var self = this;
        this.$trigger.on('input blur', function() {
            self.convert();
        });
    },
    convert: function($trigger) {
        var maxlength = this.$trigger.attr('maxlength'),
            inputValue = this.$trigger.val(),
            inputValue = inputValue.replace(/[A-Za-z0-9]/g, function(this.$trigger) {
                return String.fromCharCode(this.$trigger.charCodeAt(0) - 65248)
            }).slice(0, maxlength),
            alertText = this.getAlertText(inputValue);

        this.$trigger.val(inputValue);
        this.changeBgColor(inputValue % 5 !== 0);
        this.$output.text(alertText);
    },
    getAlertText: function(inputValue) {
        var time = Math.floor(inputValue / 60);
        var minutes = inputValue % 60;

        if (inputValue <= 0 || inputValue % 5 !== 0 || inputValue === '') {
            return '5分単位で入力してください';
        }

        if(minute) {
           minute = minute + '分';
        }
        if(hour) {
           hour + '時間';
        }
        return hour + minute
    },
    changeBgColor: function(result) {
        this.$parents.toggleClass(this.CLASS_ERROR, result);
    },
};

$(function() {
    new SAMPLE.ConvertInputTime($('. jscConvertTime'));
});
;(function(document){ var pres = document.getElementsByTagName("pre") for(var i=pres.length; i--; ){  var el = makeOl(pres[i]) pres[i].appendChild(el) } function makeOl(pre){ if (pre.className.indexOf("gist") !== -1) { return } var ol = document.createElement("ol") , li = document.createElement("li") , df = document.createDocumentFragment() , br = pre.innerHTML.match(/\n/g) || 0 ol.className = "preLine" ol.setAttribute("role", "presentation") // no lang, no line-number if( pre.className && ! /lang-./.test(pre.className) ){ br.length += 1 } for(var i=br.length; i--; ){ var li2 = li.cloneNode(true) df.appendChild(li2) } ol.appendChild(df) return ol } })(document)