Refactoring helps you grow

  • 09 December 2021
Post image

In my last article, I introduced that any beginner can implement a “voting system (front-end)”, but of course, if you Google it, copy and paste it, and then just copy and paste the code into mass production, it goes without saying that the code will be garbage. So, I’d like to refactor the code from the previous state with some advice from an imaginary senior engineer who will review it.
The completed code from last time.

<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>

Thankful flirting from senior engineers

You will probably get the following review.

  1. Why aren’t you using Vue or Svelte in the first place?
  2. Are you stupid enough to create a bunch of similar functions called buttonClickx?
  3. Do you know what Js this is?
  4. There is no error handling when sending data, though.
  5. There’s no control over consecutive button clicks, though.

Others will say “too many getElementById” or “check and control if the page has been voted on even after page reload”.

1. Knowledge is also important, of course.

First of all, why aren’t you using Vue or Svelte in the first place? If you don’t know about it, you can’t look it up. In recent years, React, Vue, Svelte, and other frameworks such as Rust and C#’s WebAssembly have become the mainstream for Javascript in browsers. It’s true that these are very useful. For communication, libraries like axios are often used, and XMLHttpRequest is rarely used as is in this code. It’s also important to keep your antennae up to collect knowledge of various things.
I’m not going to refactor the code using these frameworks.

2. Common code

In most cases, similar operations should be written in one place. The above example is just a typical duplicate code. If the code is not well standardized, it becomes very unmaintainable.

// The same role as this function has been duplicated.
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;
}

Pass this as the argument of the buttonClick function, and change the data attribute to have an id. I also changed the buttonClick function to pass this as an argument, and to have the id in the data attribute. Furthermore, I got all the elements of the button and processed them with foreach. I refactored it as follows.

<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>

Of course, if you have a lot of processes that are doing the same thing, if one of them is modified, all the others will have to be modified as well. If you have many processes that do the same thing, if one of them is modified, you have to modify all the others. In order to reduce the risk as much as possible, I’m talking about commonization in general.

3. The key is to always be able to imagine “what if…”.

If there was an insufficient implementation in the process of sending voting data after pressing the button this time, it would still be error handling. And in the first place, the data transmission by communication is thrown off. Let’s all worry about the frequent “what if this happens” of the process.
What was not taken into account this time is that, as pointed out above, what happens if the communication to send data to the API fails is missing. In addition, the fact that it is possible to press the vote button again during communication, resulting in multiple transmissions, is not controlled.

function buttonClick(elm){
  var id = elm.dataset.id
  // Deactivates all buttons at the time of button click (prohibits multiple clicks)
  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 { // In case of failure, return the button to active.
      Array.prototype.forEach.call(buttons, button => button.disabled = false)
      alert("投票に失敗しました")
    }
  }  
}

This kind of error handling, catching exceptions, null checking, etc. requires experience and familiarity. And no matter how far you go, you can’t eliminate the possibility of leaks.

Finally

If you ask me if this refactor is 100 points, it may not be. In the first place, there is no need to write such a process in normal Javascript without additional libraries nowadays. Anyway, the finished product looks like this.

<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>

But sufficiently, I think I was able to correct the missing and incorrect parts of the code in the previous article, as well as refactor it to make it more maintainable. If you have a senior engineer or a supervisor to review your work, you should listen to their irritating comments and grow. There is definitely knowledge and experience to be gained from that. If you find them annoying, just steal their skills and move on.

The refactor this time is the importance of knowing and having experience. If it were easy to learn those things, you’d have no trouble, right? But don’t worry. If you are aware of the most important process of “breaking it down into tasks and enumerating them,” you will be able to further refine this “breaking it down into tasks and enumerating them” by collecting information and getting suggestions from higher level people, and before you know it, you will have acquired them as experience.

Also, when it comes to programming knowledge, there is much less to know than what you need to know in Japanese history or world history. You don’t need too much old knowledge, just the latest mainstream is enough.

What I wanted to convey throughout was that it is most important to think and enumerate well in the beginning, and to be honest, writing programming code is mostly copying and pasting what comes up in Google searches. Then, when you refactor the deliverables, you gain new knowledge and experience, and you keep growing. The repetition of this process is what makes you a professional.

i407 - To Be Super Programmers

Written By Ason

I am a small business owner who wants to be a programmer forever. The only thing I can be proud of is that I have a good bowel movement. I am an expert at defecation.

Popular Articles

You May Also Like