5
\$\begingroup\$

Looking to improve building UI in pure ES6.

This is the pattern I have adopted and has worked fairly well. This code increments the count, sets a data attribute to toggle the button, and issues a POST to the server with the vote payload object.

import axios from "axios";

class Vote {
  constructor(el) {
    this.el       = el;
    this.url      = this.el.dataset.url;
    this.id       = this.el.dataset.id;
    this.count    = this.el.dataset.count;
    this.count_el = this.el.querySelector('span.count')
    this.setClickHandlers();
  }

  setClickHandlers() {
    this.el.addEventListener("click", ()=> {
      this.postVote();
      this.toggle();
      this.toggleCount();
    })
  }

  postVote() {
    axios({
      url: this.url,
      method: 'post',
      data: { "idea":
        {"id": this.id}
      }
    })
    .then((data) => {
      console.log(`${data}: Posted vote`);
    })
    .catch((data) => {
      console.log(data);
    })
  }

  toggle() {
    if (this.el.dataset.toggled == "true") {
      this.el.dataset.toggled = "false"
    } else {
      this.el.dataset.toggled = "true"
    }
  }

  toggleCount() {
    var current = parseInt(this.count_el.innerHTML);
    if (this.el.dataset.toggled == "true") {
      this.count_el.innerHTML = ++current;
    } else {
      this.count_el.innerHTML = --current;
    }
  }

}


document.addEventListener("turbolinks:load", ()=> {
  let votes = document.querySelectorAll('[data-vote]');
  if (votes.length) {
    votes.forEach((el) => {
      new Vote(el);
    })
  }
});
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Consistency

Sometimes you use snake_case and sometimes you use camelCase: in JavaScript we usually use camel case, it's not mandatory but stick to one. Then, for example, count_el should be countEl (or a better, more descriptive name, IMHO no need to abbreviate element here.)

In ctor you saved a copy of some properties to abbreviate (I suppose) code elsewhere however, you use it just once. You can drop it altogether.

Reuse

You parse a boolean value twice, it's enough to introduce a more robust function:

const parseBoolean(value) => (/true/i).test(value);

There are a few different techniques to parse a boolean (direct string comparison as you're doing, regex, JSON.parse(), and few more). Just pick one, in this case it's not probably that important. Note: use === instead of ==.

Now that you have a function (which accepts, via conversion, both a boolean and a string) you can write:

toggle() {
    this.el.dataset.toggled = !parseBoolean(this.el.dataset.toggled);
}

In this example I'm storing boolean, add .toString() if appropriate. toggleCount() will then use if (parseBoolean(this.el.dataset.toggled)). You may want to introduce a function const isElementToggled = () => parseBoolean(this.el.dataset.toggled);.

Style & misc

IMHO setClickHandlers() does both too little and too much. Its responsibility is to add a click handler (and it's too little to be a separate function) but it also IS the click handler (and it's too much). Declare a separate function:

handleClick(e) {
    this.postVote();
    this.toggle();
    this.toggleCount();
}

And attach it directly in your ctor:

el.addEventListener("click", this.handleClick.bind(this));

If you're targeting ES6 then do not use (unless there is a strong compelling reason) var, use const and let instead.

The snippet where you create Vote may be simplified, no need to check Array.length if using Array.forEach():

document.addEventListener("turbolinks:load", ()=> {
  document.querySelectorAll('[data-vote]').forEach(x => new Vote(x));
});

If the AJAX call to post a vote fails then you local counter is already incremented and it won't go back. The problem is that you consider these actions as part of the click event handler when they should really be an effect of the sucessfull AJAX call:

axios({ ... }).then((data) => {
    this.toggle();
    this.toggleCount();
});

In this way if the call fails you won't have unsynchronized UI. Alternatively, if call is really slow, you may want to update UI immediately and revert it back if failed:

this.toggle();
this.toggleCount();

axios({ /* ... */ }).then((data) => {
    // ...
}).catch((data) => {
    this.toggle();
    this.toggleCount();
});

In any case the code to attach the event handler in ctor will then simply be:

el.addEventListener("click", this.postVote.bind(this));

I, personally, find that to create a discardable object only because of its side effects is slightly misleading for the reader. In JavaScript you can have nested functions and if you do not need to carry a state then you do not need classes. There is no reason to avoid something like this:

document.addEventListener("turbolinks:load", ()=> {
  document.querySelectorAll('[data-vote]').forEach(attachClickHandler);
});

Where attachClickHandler() is simply:

function attachClickHandler(element) {
    function postVote() { /* ... */ }
    function toggle() { /* ... */ }
    function toggleCount() { /* ... */ }

    el.addEventListener("click", postVote);
}

Code might be even more simple if you had a module for this "Vote Component" but it really really depends on the UI framework/library you're using (if any.)

\$\endgroup\$
1
  • \$\begingroup\$ Excellent answer. Thanks for taking the time. Do you mean module pattern? I suppose I was using a class as an organizational structure and it may be unneeded. Thanks again. \$\endgroup\$
    – cpk
    Aug 6, 2018 at 21:48
2
\$\begingroup\$

In addition to Adriano Repetti answer I would like to add some additional points.

  • You should not include debugging code in your release code.

Your postVote function

postVote() {
    axios({
        url: this.url,
        method: 'post',
        data: { "idea": { "id": this.id  }}
    })
    .then((data) => {
        console.log(`${data}: Posted vote`); // << this is a debug output
    })
    .catch((data) => {
        console.log(data);  // << this is a debug output
    })
}

The two calls to console are in my opinion rude. Only output to a console you have access to IE your development platforms, and treat the clients console as a private out of bounds area.

  • You don't need to wrap property names in quotes unless the names are not valid JS names eg "some-prop" : data,, and it is also inconsistent with the rest of your code that does not do this.

From above snippet

 data: { "idea": { "id": this.id  }}
 // should be 
 data: { idea: { id: this.id  }}  
  • Code style is up to the individual but be constant in the style you use. You are using semicolons for some lines yet not on others. Use them on all lines that invoke automatic semicolon insertion (preferably) or on none, don't mix.
\$\endgroup\$
2
  • \$\begingroup\$ Thanks. Do you recommend semicolons or no? \$\endgroup\$
    – cpk
    Aug 6, 2018 at 21:47
  • 1
    \$\begingroup\$ @cpk I come from a C background and missing semicolons just look wrong. In JS its not so much which you use, its understanding what problems the missing semicolon can cause, if you are unsure then the safest option is to include them. It is always a good idea to check code using a linter/hinter to catch overlooked inconsistencies in your preferred style. \$\endgroup\$
    – Blindman67
    Aug 7, 2018 at 1:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.