HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

OOP and non-OOP versions of layout and page animation code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
animationnonlayoutcodepageversionsandoop

Problem

I am trying to learn how to write more object oriented javascript code, but I am having a hard time understanding the correct way to do it, and how it is useful for front end development. As you can see below, I tried to rewrite my old javascript code to be more object oriented, but it just doesn't seem necessary for what I am doing. I am not even sure if I am doing it correctly (would it be better to use a self executing anonymous function for this?). Could someone take a look at my code and tell me how I can improve it? Also, explain to me why it is important to write object oriented javascript for the development of a websites frontend?

Original Code

// Document and Windows Variables
var $document = $(document),
    $window = $(window);

// Equalize
function equal_heights() {
    var $row = $('.equal');

    if ($row.length > 0) {
        $row.each(function() {
            var $sections = $(this).children(),
                largest_height = 0;

            $sections.height('auto');

            $sections.each(function() {
                var h = $(this).height();
                if (largest_height  $(this).offset().top - ($window.height() / 1)) {

            $(this).addClass('is-showing');

        }
    });
}

page_animations();

/********************
Run Functions
********************/
$window.on({
  load: function() {
   equal_heights();
   position_popup();
  }
});


OOP

```
// Document and Windows Variables
var $reuseVars = {
window: $(window),
document: $(document)
}

// Functions Object
function AllFunctions() {}

AllFunctions.prototype = {
// Equal Heights
equalHeights: function() {
var $row = $('.equal');

if ($row.length > 0) {
$row.each(function() {
var $sections = $(this).children(),
largest_height = 0;

$sections.height('auto');

$sections.each(function() {
var h = $(this).height();
if (la

Solution

I think you are over-engineering here. OOP is not an end in itself, so don't convert your code to "OOP style" just because.

The most desirable property of code (after correctness) is clarity. Clarity means obviousness, which means ease of maintenance, both of which mean reduced chance of bugs. To me, your second code sample is less clear than your first.

I would go into a different direction and just try to simplify your first code sample instead of slapping some fancy "OOP" stuff on.

Part of simplification is abstracting functionality into re-usable blocks. jQuery is very easy to extend with your own functions, so let's create two extensions maxHeight and positionCenter:

$.fn.extend({
    maxHeight: function () {
        var heights = this.map(function() { return $(this).height(); });
        return Math.max.apply(null, heights);
    },
    positionCenter: function () {
        return this.each(function () {
            var $this = $(this),
                height = $this.outerHeight(true),
                width = $this.outerWidth(true),
                browser_height = $(window).height(),
                top = 10,
                page_pos = document.documentElement.scrollTop || document.body.scrollTop;

            if (height < browser_height) {
                top = browser_height / 2 - height / 2;
            }
            $this.css({
              top: page_pos + top
              // "left" calculation is still missing
            });
        });
    }
});


This could end up in a script file where you collect all your custom extensions.

With some of the plumbing out of the way, the code that actually works on the page becomes a whole lot clearer:

$(window).scroll(function () {
    var $window = $(window);
    $('.fade-me').each(function() {
        var $this = $(this);
        if ($window.scrollTop() > $this.offset().top - $window.height() / 1) {
            $this.addClass('is-showing');
            // when is this class removed?
        }
    });
});

$(function () {
    $('.equal').each(function() {
        var $sections = $(this).children().height('auto');
        $sections.height( $sections.maxHeight() );
    });
    $('.popup').positionCenter();
});


(The above is of course untested, you might have to tweak it a little here and there.)

The next thing you could investigate is making your own custom jQuery selectors.

Imagine your code scroll handler would look like this:

$(window).scroll(function () {
    $('.fade-me:inview').addClass('is-showing');
});


Wouldn't that be even clearer?

Here is how to do that: http://www.sitepoint.com/make-your-own-custom-jquery-selector/

As a finger exercise, write a jQuery extension named equalizeHeight, such that you can make the following simplification:

$('.equal').each(function() {
    $(this).children().equalizeHeight();
});

Code Snippets

$.fn.extend({
    maxHeight: function () {
        var heights = this.map(function() { return $(this).height(); });
        return Math.max.apply(null, heights);
    },
    positionCenter: function () {
        return this.each(function () {
            var $this = $(this),
                height = $this.outerHeight(true),
                width = $this.outerWidth(true),
                browser_height = $(window).height(),
                top = 10,
                page_pos = document.documentElement.scrollTop || document.body.scrollTop;

            if (height < browser_height) {
                top = browser_height / 2 - height / 2;
            }
            $this.css({
              top: page_pos + top
              // "left" calculation is still missing
            });
        });
    }
});
$(window).scroll(function () {
    var $window = $(window);
    $('.fade-me').each(function() {
        var $this = $(this);
        if ($window.scrollTop() > $this.offset().top - $window.height() / 1) {
            $this.addClass('is-showing');
            // when is this class removed?
        }
    });
});

$(function () {
    $('.equal').each(function() {
        var $sections = $(this).children().height('auto');
        $sections.height( $sections.maxHeight() );
    });
    $('.popup').positionCenter();
});
$(window).scroll(function () {
    $('.fade-me:inview').addClass('is-showing');
});
$('.equal').each(function() {
    $(this).children().equalizeHeight();
});

Context

StackExchange Code Review Q#125586, answer score: 3

Revisions (0)

No revisions yet.