リファクタリングが自分を成長させてくれる

　前回の記事では、どんな初心者でも「投票システム（フロント）」を実装できるっていうことを紹介したんだけど、当然ググってコピペ、そのコードをひたすらコピペで量産では、ごみコード（またの名をウンコード）になるのはいうまでもない。なので前回の状態からレビューをしてくれる架空の先輩エンジニアからのアドバイス的なものでリファクタしていきたい。
　前回の完成形コード。

<div class="contents">
  <div>
    <h5>ドラえもん</h5>
    <p>投票数：<span id="target1" class="count">0</span></p>
    <button id="button1" onclick="buttonClick1()">投票</button>
  </div>
  <div>
    <h5>アンパンマン</h5>
    <p>投票数：<span id="target2" class="count">0</span></p>
    <button id="button2" onclick="buttonClick2()">投票</button>
  </div>
  <div>
    <h5>カカロット</h5>
    <p>投票数：<span id="target3" class="count">0</span></p>
    <button id="button3" onclick="buttonClick3()">投票</button>
  </div>
</div>

<script>
window.onload = function(){
  var data_url = new XMLHttpRequest();
  data_url.open('GET', 'https://i-407.com/'); //このURLは実際のAPIURLにする必要がある
  data_url.send();
  data_url.onreadystatechange = function() {
    // 今回はAPI実装はないので、ここは仮のランダム値を入れる
    document.getElementById("target1").innerText = Math.floor(Math.random() * 11);
    document.getElementById("target2").innerText = Math.floor(Math.random() * 11);
    document.getElementById("target3").innerText = Math.floor(Math.random() * 11);
  }
}
function buttonClick1(){
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=1');
  var cnt = document.getElementById("target1").textContent;
  document.getElementById("target1").innerText = parseInt(cnt) + 1;
  document.getElementById("button1").disabled = true;
  document.getElementById("button2").disabled = true;
  document.getElementById("button3").disabled = true;
}
function buttonClick2(){
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=2');
  var cnt = document.getElementById("target2").textContent;
  document.getElementById("target2").innerText = parseInt(cnt) + 1;
  document.getElementById("button1").disabled = true;
  document.getElementById("button2").disabled = true;
  document.getElementById("button3").disabled = true;
}
function buttonClick3(){
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=3');
  var cnt = document.getElementById("target3").textContent;
  document.getElementById("target3").innerText = parseInt(cnt) + 1;
  document.getElementById("button1").disabled = true;
  document.getElementById("button2").disabled = true;
  document.getElementById("button3").disabled = true;
}
</script>

先輩エンジニアからの有難いイチャモン

　おそらく以下のようなレビューを受けるだろう。

そもそもなんでVueとかSvelte使ってないの？
buttonClickxっていう同じような関数たくさん作って頭悪いの？
Jsのthisって知ってる？
データ送信時のエラーハンドリングが出来ていないけど
ボタンの連続クリックが制御されてないけど

　他にも「getElementById多すぎ」とか「ページリロード後にも既投票かチェックして制御しろ」とか言ってくるだろう。

1. 知識も当然重要

　まずそもそもなんでVueとかSvelte使ってないの？ って言われても知らないもんは調べようもない。ブラウザのJavascriptは近年React,Vue,Svelte、他にはRustやC#のWebAssemblyといったフレームワークが主流だ。たしかにこれらは便利だからね。あと、通信のためのライブラリはaxiosとかがよく使われていて、本コードのようにXMLHttpRequestをそのまま使用することは少ない。いろんな知識を収集するアンテナを貼っておくことも重要なことだ。
　ただ今回これらのフレームワークでのリファクタはしない。

2. コードの共通化

　ほとんどの場合、同じような処理は１か所で記述すべきなんだ。上記のような例はまさに典型的な複製コードだね。うまくコードを共通化できないと保守性が非常に低くなる。

// この関数と同じ役割のものが複製されている
function buttonClick1(){
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=1');
  var cnt = document.getElementById("target1").textContent;
  document.getElementById("target1").innerText = parseInt(cnt) + 1;
  document.getElementById("button1").disabled = true;
  document.getElementById("button2").disabled = true;
  document.getElementById("button3").disabled = true;
}

　buttonClick関数の引数にthisを渡すのと、data属性でidを持たせるように変更。さらにボタンのエレメントとかは全部取得してforeachで処理するとか。以下のようにリファクタしてみた。

<button onclick="buttonClick(this)" data-id="1">投票</button>
<script>
function buttonClick(elm){
  var id = elm.dataset.id
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=' + id);
  var spans = elm.parentNode.getElementsByTagName("span");
  spans.length > 0 ? spans[0].innerText = parseInt(spans[0].textContent) + 1 : null
  var buttons = document.getElementById("app").getElementsByTagName("button");
  Array.prototype.forEach.call(buttons, button => button.disabled = true)
}
</script>

　当たり前だが、同じようなことをしている処理がたくさんあると、そのうち一つの修正が合った場合、他の全ても修正しないといけない。そのときに本当に修正漏れがないと確実に言えますかってこと。少しでもリスクを下げるために、一般的には共通化しようって話。

3. 常に「もしも～だったら」を想像できるかがポイント

　今回のボタンを押してからの投票データ送信処理に不十分な実装があったとすると、やはりエラーハンドリングだろう。そもそも通信によるデータ送信が投げっぱなしになっているし。みなさんは、常に処理の『もしこんなことが起こったら』と頻繁にビクビクして心配しよう。
　今回考慮が欠けていたのは、上記の指摘にある通り、APIへのデータ送信の通信が失敗した場合どうするかが抜けている。また加えて、通信中に投票ボタンを再度押すことができるので多重送信になってしまうことが制御できていない。

function buttonClick(elm){
  var id = elm.dataset.id
  // ボタンクリックと当時にすべてのボタンを非活性にしてしまう（多重クリック禁止）
  var buttons = document.getElementById("app").getElementsByTagName("button");
  Array.prototype.forEach.call(buttons, button => button.disabled = true)

  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=' + id);
  send_data.onreadystatechange = function() {
    if (data_url.status === 200) { //通信成功時
      var spans = elm.parentNode.getElementsByTagName("span");
      spans.length > 0 ? spans[0].innerText = parseInt(spans[0].textContent) + 1 : null
    } else { // 失敗時にはボタンを活性に戻す
      Array.prototype.forEach.call(buttons, button => button.disabled = false)
      alert("投票に失敗しました")
    }
  }  
}

　このようなエラーハンドリングや例外処理のキャッチ、その他nullチェックなどは、経験・慣れが必要だ。そしてどこまでいっても漏れてしまう可能性は排除できないものだ。

最後に

　今回のこのリファクタが100点かと言われたらそうではないかもしれない。そもそも今時、追加ライブラリ無しのノーマルなJavascriptでこのような処理を書く必要すらない。とにかく完成形は以下のような形になった。

<div id="app" class="contents">
  <div>
    <h5>ドラえもん</h5>
    <p>投票数：<span class="count">0</span></p>
    <button onclick="buttonClick(this)" data-id="1">投票</button>
  </div>
  <div>
    <h5>アンパンマン</h5>
    <p>投票数：<span id="target2" class="count">0</span></p>
    <button onclick="buttonClick(this)" data-id="2">投票</button>
  </div>
  <div>
    <h5>カカロット</h5>
    <p>投票数：<span id="target3" class="count">0</span></p>
    <button onclick="buttonClick(this)" data-id="3">投票</button>
  </div>
</div>
<script>
function buttonClick(elm){
  var id = elm.dataset.id
  var buttons = document.getElementById("app").getElementsByTagName("button");
  Array.prototype.forEach.call(buttons, button => button.disabled = true)
  var send_data = new XMLHttpRequest();
  send_data.open('POST', 'https://i-407.com/');
  send_data.setRequestHeader('content-type', 'application/x-www-form-urlencoded');
  send_data.send('id=' + id);
  send_data.onreadystatechange = function() {
    if (data_url.status === 200) {
      var spans = elm.parentNode.getElementsByTagName("span");
      spans.length > 0 ? spans[0].innerText = parseInt(spans[0].textContent) + 1 : null
    } else { 
      Array.prototype.forEach.call(buttons, button => button.disabled = false)
      alert("投票に失敗しました")
    }
  }  
}
</script>

　しかし十分に、前回の記事のコードに足りない部分、誤っていた部分の修正、また、保守性を高めるリファクタリングができたと思う。もし先輩エンジニアや上司などのレビューをしてくれる人がいる場合は、彼らのイラつく上から目線の小言も聞き入れて成長すべきだ。そこから得られる知識や経験は絶対存在するからね。ただ人間的にウザイのであれば、その人から技術を盗むだけ盗んで転職しちゃえばいい。

　今回のリファクターは「知っているか否か」、「経験があるか否か」という点が重要になっている。それらを簡単に身につけられたら苦労しないって？でも大丈夫。「タスクに分解して列挙」という一番大切な工程を意識できていれば、よりレベルの高い人からの指摘や、情報収集によってこの「タスクに分解して列挙」がさらに洗練されていき、気が付いたらそれらが経験として身になっているから。

　また、プログラミングの知識っていうけど、はっきり言って「日本史」とか「世界史」の勉強よりはるかに知っておくべき知識は少ないぜ？古すぎる知識はほとんど必要ないし、最近の主流とかだけでも十分じゃん。

　全体を通して伝えたかったのは、はじめにしっかり考えて列挙することが最も大切で、プログラミングコードを書くのは正直ほとんどがGoogle検索で出てくるもののコピペ。そして、成果物をリファクタするときに新しい知識や経験を身につけていくことでどんどん成長していく。この一連のながれの繰り返しがあなたをプロにするってこと。