4
\$\begingroup\$

I'm studying JavaScript and jQuery and I did an implementation of a simple cat clicker game:

class Cat {
  constructor(name, picture) {
    let self = this;

    self.name = name;
    self.picture = picture;
    self.count = 0;

    let mainDiv = $('#cats');
    let catDiv = $('<div></div>');

    mainDiv.append(catDiv);

    self.img = $(`<img src="${self.picture}">`);
    catDiv.append(self.img);

    self.counter = $('<p></p>');
    catDiv.append(self.counter);

    self.img.click(function() {
      self.count++;
      self.counter.text(`You clicked ${self.name} ${self.count} times.`);
      self.counter.css("font-size", "+=0.5");
    });
  }
}

$(document).ready(function() {
  new Cat('Kimmy', 'http://newmediarockstars.com/wp-content/uploads/2014/07/femcat.png');
  new Cat('Logan', 'http://svoctopussy.com/wp-content/uploads/2013/02/Grumpy-Cat.jpg');
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.0.0/jquery.min.js"></script>
<h1>Cat Clicker</h1>
<div id="cats"></div>

I wanted it to be dynamic, so if I want to add more cats, I just have to instance a new object of the Cat class, like it's done in the document.ready function.

The Cat constructor adds a new cat in the HTML (it appends the cats div), using div, h1, img and p. Everytime you click the img (🐱), the p element text is updated with the click count.

I'm a complete beginner in JS. What can I refactor here to make the code more legible and idiomatic?

EDIT: In the third line, there's a let self = this;. I did that because I needed to access the Cat instance inside the img.click callback function. I don't know if that's how it's done nor how I can improve that.

\$\endgroup\$
  • 3
    \$\begingroup\$ I see you are hotlinking to images on other websites. That's great for training purposes, but before you publish it anywhere, be sure to point to local resources. \$\endgroup\$ – Nix Sep 7 '17 at 12:44
3
\$\begingroup\$

In the third line, there's a let self = this;. I did that because I needed to access the Cat instance inside the img.click callback function.

Use an arrow function, which won't rebind this. The self alias can then be removed.

this.img.click = () => {
  this.count++;
  ...
}

Your examples don't save the Cats anywhere, access any properties on them, pet them, or otherwise interact with them -- if adding a cat to the DOM is all you need, the constructor doesn't need to attach anything to this (which is equivalent to exposing things as public properties).

The Cat constructor adds a new cat in the HTML (it appends the cats div), using div, h1, img and p.

Putting that behaviour in the constructor is not ideal. People reading your code probably expect new Cat(...) to return a cat, but not necessarily for it to put that cat somewhere. Besides being unexpected, this isn't very flexible: what if I want to put a cat somewhere else on the page?

It's probably better to separate creating the cat (and its click handler) from actually attaching it anywhere. You could add another method in the Cat class, like this:

  attach(element) {
    element.append(this.catDiv);
  }

but I think all you need in this example is a function:

function createCat(name, picture) {
  let clickCount = 0;

  const $cat = $('<div/>');    // $var is just a naming convention for jQuery variables
  const $img = $('<img/>', { src: picture }).appendTo($cat);
  const $counter = $('<p/>').appendTo($cat);

  $img.click(() => {
    clickCount++;
    $counter
      .text(`You clicked ${name} ${clickCount} times.`)
      .css("font-size", "+=0.5");    // Most jQuery methods can be chained
  });

  return $cat;    // return the cat div, not yet attached to the DOM
}


$(function() {    // equivalent to $(document).ready(function() {
  const $main = $('#cats');

  $main.append(createCat('Kimmy', 'http://newmediarockstars.com/wp-content/uploads/2014/07/femcat.png'));
  $main.append(createCat('Logan', 'http://svoctopussy.com/wp-content/uploads/2013/02/Grumpy-Cat.jpg'));
});
\$\endgroup\$
  • \$\begingroup\$ That's great, thank you @BenC. Why did you change let to const in the $cat, $img and $counter? Is there any site with all those code conventions? \$\endgroup\$ – vnbrs Sep 7 '17 at 11:39
  • 2
    \$\begingroup\$ @vnbrs per the MDN documentation for const: "Constants are block-scoped, much like variables defined using the let statement. The value of a constant cannot change through re-assignment, and it can't be redeclared.". This is appropriate for things like the DOM elements (e.g. $('#cats') since those shouldn't change \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 7 '17 at 13:17
  • 1
    \$\begingroup\$ let works fine too. I prefer to make everything const unless it needs to be reassigned, so that reassignable variables are more obvious. But that's more of a style choice. \$\endgroup\$ – BenC Sep 7 '17 at 16:41
1
\$\begingroup\$

BenC has provided some great pointers. I will attempt to expand one implicit change and mention a couple other possible changes.

DOM-ready function change

To be explicit about one thing changed in BenC's answer, $(document).ready(function() { was changed to $(function() { because the documentation for .ready() states:

jQuery offers several ways to attach a function that will run when the DOM is ready. All of the following syntaxes are equivalent:

  • $( handler )
  • $( document ).ready( handler )
  • $( "document" ).ready( handler )
  • $( "img" ).ready( handler )
  • $().ready( handler )

As of jQuery 3.0, only the first syntax is recommended; the other syntaxes still work but are deprecated.1

Your sample code utilizes the 2.0 version of jQuery. Perhaps this is because the snippet lists 2.0 as the highest despite 3.1.1 being available in the same CDN, but if you are still using 2.0, it might be beneficial to upgrade to 3.0+ to take advantage of newer features.

Closure/lambda function scope on click

BenC mentioned you could use an arrow function and not have to make a separate variable self to point to this in the Cat constructor. Another approach is to use Function.bind(). As is illustrated in the code below, call .bind() after the closing curly-bracket and pass this:

this.img.click(function() {
    this.count++;
    this.counter.text(`You clicked ${this.name} ${this.count} times.`);
    this.counter.css("font-size", "+=0.5");
}.bind(this));

See this illustrated in the sample below.

Accessing container div from DOM

BenC re-wrote the code to access the container (i.e. $('#catDiv')) only once and went so far as to eliminate the Cat class. If you wanted to keep the Cat class, you could still assign that DOM reference once in the DOM ready callback. I would suggest declaring the variable var catDiv outside the Class and DOM-ready callback:

var catDiv;
Class Cat {

Then it can be assigned in the DOM-ready callback:

$(function() { //equivalent to DOM-ready
    //now that the DOM is ready, set catDiv here instead of each time Cat constructor is called
    catDiv = $('<div></div>');

And then it can be referenced with-in the constructor to add the image and counter elements. If the value was assigned at the time of declaration, then const could be used instead of var.

//declare catDiv here so it can be used in the class and DOM ready callback
var catDiv;
class Cat {
  constructor(name, picture) {
    this.name = name;
    this.picture = picture;
    this.count = 0;

    let mainDiv = $('#cats');
    mainDiv.append(catDiv);

    this.img = $(`<img src="${this.picture}">`);
    catDiv.append(this.img);

    this.counter = $('<p></p>');
    catDiv.append(this.counter);

    this.img.click(function() {
      this.count++;
      this.counter.text(`You clicked ${this.name} ${this.count} times.`);
      this.counter.css("font-size", "+=0.5");
    }.bind(this));
  }
}

$(function() { //equivalent to DOM-ready
  //now that the DOM is ready, set catDiv here instead of each time Cat constructor is called
  catDiv = $('<div></div>');
  new Cat('Kimmy', 'http://newmediarockstars.com/wp-content/uploads/2014/07/femcat.png');
  new Cat('Logan', 'http://svoctopussy.com/wp-content/uploads/2013/02/Grumpy-Cat.jpg');
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<h1>Cat Clicker</h1>
<div id="cats"></div>


1http://api.jquery.com/ready/

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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