Adapting new design pattern with post handler

Question

I'm currently using this pattern in my JavaScript code. Somewhere in my code I use Dnianas.Post.init. init fires and it will bind DOM events.

At first everything was OK. But then, when you look at likePost method you will see that, I'm not using the var keyword to declare my variables. Because I if do that, then it won't be available to renderCreatedPost method. So basically I'm forced to do that.

I'm trying to adapt a new pattern and refactor my code that allow for that.

As you may know, defining variables without the var keyword is a bad practice and will result in errors in strict mode.

Dnianas.Post = {

    init: function () {
        this.bindEvents();
    },

    bindEvents: function () {
        $(document).on('submit', '#postForm', this.createPost);
        $(document).on('click', '#likePost', this.likePost);
    },

    createPost: function (event) {
        var self = Dnianas.Post;

        var request = $.ajax({
            url: '/posts/create',
            data: $(this).serialize(),
            beforeSend: function () {
                $loading.show();
            },
            complete: function () {
                $loading.hide();
            }
        });

        request.done(function (data) {
            self.renderCreatedPost(data);
        });

        event.preventDefault();
    },

    renderCreatedPost: function (data) {
        if (data.success == 'false') {
            $('.error').empty().append(data.message).slideDown(100).delay(3000).slideUp(250);
        } else {
            this.postForm[0].reset();
            $(data.post_html).insertAfter(this.postForm).hide().delay(100).slideDown(400);
            $('.no-posts').hide();
        }
    },

    likePost: function () {
        self = Dnianas.Post;
        $likePostEl = $(this);
        $postLikeCountEl = $likePostEl.children('#likeCount');
        postLikeCount = $postLikeCountEl.data('count');

        var request = $.ajax({
            url: '/posts/like',
            data: {
                post_id: $likePostEl.parents().get(2).dataset.id,
                user_id: $likePostEl.parents().get(2).dataset.userid,
                like_count: postLikeCount,
                _token: token
            }
        });

        request.done(function (data) {
            self.renderPostLike(data);
        });
    },

    renderPostLike: function (data) {
        if (data.like) {
            $likePostEl.children('.likeIcon').addClass('liked');
            $postLikeCountEl.addClass('liked');
        } else {
            $likePostEl.children('.likeIcon').removeClass('liked');
            $postLikeCountEl.removeClass('liked');
        }
        $postLikeCountEl.text(data.like_count);
    },

    getMaxId: function($element) {
        // Filter through the ids
        var ids = $element.map(function () {
            return +$(this).data('id') || 0;
        });

        // Get the highest id attribute
        last_id = Math.max.apply(Math, ids);

        // Return the maximum id.
        return last_id;
    },

    getNewPosts: function () {
        var self = Dnianas.Post;
        var ids = [];
        var $postEl = $('.poster_memb .tab_post');
        last_id = 0;

        // If there was posts.
        if ($postEl.length > 0) {
            self.getMaxId($postEl);
        }

        var request = $.ajax({
            url: '/posts/latest/' + last_id,
            type: 'GET',
            dataType: 'JSON'
        });

        request.done(function (data) {
            self.renderLatestPost(data);
        });
    },

    renderLatestPost: function (data) {
        if (data && data.is_new == 'true' && data.html) {
            $('.no-posts').hide();
            $(data.html).insertAfter('#postForm').hide().slideDown(500);
        }

        // Run the function every 10 seconds.
        setTimeout(this.getNewPosts, 10000);
    }
};

Answer 1

@GaryStorey hit on the simplest solution: Just pass your variables to the function:

// ...

likePost: function () {
    var $likePostEl = $(this),
        $postLikeCountEl = $likePostEl.children('#likeCount'),
        postLikeCount = $postLikeCountEl.data('count');

    var request = $.ajax({
        url: '/posts/like',
        data: {
            post_id: $likePostEl.parents().get(2).dataset.id,
            user_id: $likePostEl.parents().get(2).dataset.userid,
            like_count: postLikeCount,
            _token: token
        }
    });

    request.done(function (data) {
        Dnianas.Post.renderPostLike(data, $likePostEl);
    });
},

renderPostLike: function (data, $postEl) {
    var $likeCountEl = $postEl.children('#likeCount'),
        $likeIconEl = $postEl.children('.likeIcon');

    $postEl.toggleClass('liked', data.like);
    $likeIconEl.toggleClass('liked', data.like);
    $likeCountEl.text(data.like_count);
},

// ...

You'll note a few more changes:

• There's no real need to pass all the variables. It seems everything can be found via the post element. So just find 'em.

• Using .toggleClass() with a boolean as its 2nd argument is simpler than if/else branching

• I've removed self for 2 reasons:

  1. it's confusing that self isn't used to make a closure reference to this but to something else entirely
  2. It's not that hard to just write Dnianas.Post. instead of self.

Looking at your code though, I suspect you have some flaky HTML and CSS too.

1. Why is it necessary to set the class liked on both the post element, and one of its children (the like icon)? Your CSS should just have a rule like post.liked .likeIcon { ... } so you only need to set the class on the post element, and the icon automatically changes.

2. The like-count element should probably not have that ID. Element IDs are meant to be unique across the whole page. I imagine you probably have several posts on a page, which would mean you have multiple elements with the ID #likeCount - and that's bad! You're doing the right thing with the like icon: It has a class name. And, contrary to IDs, class names are meant to be repeated.

Also, this stuff:

$likePostEl.parents().get(2).dataset.id

is just very, very fragile and opaque. You're getting the... um, the id property of the dataset of the great-grandparent of the post-like-widget element? That's pretty abstract. What is that element? I'm guessing it's the overall post container element, but it might be <body> or some random <div> used for layout. Are you sure the great-grandparent will always be the right element? What if the like-widget is wrapped in a different structure, and the great-grandparent is no longer what you expect it to be? And why does the great-grandparent have that data?

If I'm not mistaken, the two pieces of data you're fetching are the post author's ID, and the post's own ID. Those should rather be set on the like-button element itself, so it's where it's needed.

Or you should use a selector with .parents(), like .parents(".postContainer") or something. That would make the code a lot more understandable, and make the markup more flexible.

And why are you using dataset instead of jQuery's .data() which has better browser support and does the same thing? Edit: Oh, right, it's using .get() which returns a "naked" DOM element without the jQuery wrapper. The alternative would be ....eq(2).data("id"), since .eq() returns jQuery-wrapped elements.

But I'm also wondering why you're sending the like-count to the server. The server presumably knows the count, and is sure to be up-to-date unlike the page you're looking at. The server presumably also knows the user ID (whether it's the current user's ID, or the post author's ID). All you should need to send is the post's ID.