5
\$\begingroup\$

I have a parent object (a display window) and it has a few child objects (buttons and controls on the window). I would like one child object to hide itself when another is clicked. Currently, I have my parent object bind to a custom event expected to be fired from the child object. Then, the parent object passes the command on to its other child.

Example:

//Namespace for the 'Orders' page.
$(function Orders() {
    "use strict";
    var orders = $('#Orders');

    var ordersSearchResult = new OrdersSearchResult();
    var ordersSearchDisplay = new OrdersSearchDisplay();
    var ordersEditDisplay = new OrdersEditDisplay();

    ordersSearchResult.selector.bind('searchResultLinkClicked', function () {
        ordersEditDisplay.hide();
        ordersSearchDisplay.fadeIn();
    });
});

I have two questions / concerns about this code:

  • I expose a property called 'selector' for ordersSearchResult. This is a direct reference to the jQuery DOM element. As I understand it this is bad practice -- objects should expose methods to affect their DOM elements, but not the DOM elements themselves. Previously, I passed the onSearchResultLinkClicked anonymous function into a method of ordersSearchResult which bound the event to the selector internally. I felt that that implementation was counter-intuitive, though, as it did not read very clearly to see anonymous functions being passed into 'binding' methods.
  • ordersEditDisplay and ordersSearchDisplay expose methods hide() and fadeIn() which affect their DOM elements, but ordersEditDisplay/ordersSearchDisplay are not DOM elements themselves. Is it bad practice to 'confuse' other developers like this? The code could read: ordersEditDisplay.selector.hide() but I did not want to expose the selector publically.
\$\endgroup\$
3
\$\begingroup\$

It's really hard to judge if exposing selector is the "right" thing to do without reviewing more of your source code. Also, it depends on your architecture design. If you're using a Model View Controller approach, then yes. Elements associated with the view will be exposed. Check out some of the examples for Backbone.js.

Here are some other factors that come into play:

  • Coding standards
  • size of your source
  • how strictly you're required to follow Object Oriented Programming, OOP.
  • who will maintain your code.

The code you provided looks great. As long as you produce consistent readable code that has a good level of data abstraction and works, then you shouldn't worry about the small details.

If you're still concerned about it though then just create an adapter for the .bind().

Old Code:

ordersSearchResult.selector.bind('searchResultLinkClicked', function () {
    ordersEditDisplay.hide();
    ordersSearchDisplay.fadeIn();
});

New Code:

...
OrdersSearchResult.prototype.bind = function(eventName, fn ){
    this.selector.bind( eventName, fn );
};
...
var ordersSearchResult = new OrdersSearchResult();
ordersSearchResult.bind('searchResultLinkClicked', function () {
    ordersEditDisplay.hide();
    ordersSearchDisplay.fadeIn();
}); 

Here are a few additional tips.

1) Name variables and properties based on their overall purpose and type.

Programmer A: There's a problem with orders in your code.
Programmer B: Which one? Are you talking about the function, jQuery object, element id, or something else?

To avoid this situation, create unique names assuming the language is case insensitive.

Old code:

$(function Orders() {
    "use strict";
    var orders = $('#Orders');
    ...

New code:

$(function OrdersFunc() {
    "use strict";
    var $orders = $('#OrdersTable');
    ...

More info here: "How to name variables"

2) Add the dollar sign as a prefix for jQuery variables.

It would make more sense to rename selector as $el.

3) Try not to create method names that are the same from a different library API.

ordersEditDisplay.hide() sounds like ordersEditDisplay returns a jQuery object, but that isn't the case. Instead of ordersEditDisplay.hide();, try ordersEditDisplay.hideView();

4) Use the Singleton Pattern or an object literal to create a namespace.

Great examples can be found here

Final Code:

var mySite = {};
mySite.Orders = {};
mySite.Orders.init = function() {
    "use strict";
    var $orders = $('#OrdersTable');

    var ordersSearchResult = new OrdersSearchResult();
    var ordersSearchDisplay = new OrdersSearchDisplay();
    var ordersEditDisplay = new OrdersEditDisplay();

    ordersSearchResult.$el.bind('searchResultLinkClicked', function () {
        ordersEditDisplay.hideView();
        ordersSearchDisplay.fadeInView();
    });
};
$(mySite.Orders.init);
\$\endgroup\$
  • \$\begingroup\$ I appreciate your thorough response. Parts of it I have applied to my code, but I can't help but feel that the final code has changed stylistically, but not structurally. While it is more clear that a jQuery element is exposed -- the true issue hasn't been resolved. Is it standard to expose jQuery elements publicly from objects? It does not seem that way to me. I think the hideView over hide suggestion is a very strong improvement. Naming standards are always tricky, but I definitely appreciate the points you've made. \$\endgroup\$ – Sean Anderson Sep 27 '12 at 20:25
  • 1
    \$\begingroup\$ @SeanAnderson Updated my answer. \$\endgroup\$ – Larry Battle Sep 27 '12 at 21:39

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.