2
\$\begingroup\$

I'm a hacker, not a trained engineer. I ask/answer specific questions on StackOverflow and am pretty confident in my ability to get the browser to do what I want, I thought I could benefit a great deal from a code review- hopefully I'm in the right place.

Please tear down my bright-eyed naivetie with cutting comments on code style, design, etc if I am and let me know if this isn't what this site is for.

This is a filter class that hides and shows long lists of content in container divs based on the content meta data. A sample content div:

<div class="todd.packer StreamContent" 
  data-username="todd.packer" 
  data-topic_id="13" 
  data-post_id="1394">
  ...
</div>

The class itself (relies on JQuery):

function StreamFilter(_containerId, _testAttribute, _targetStyleClass, _applyStyleClass, _testFor){
    return{
        active : false,
        filterHash : {},
        testFor : _testFor,
        CONTAINER_ID : _containerId,
        TEST_ATTRIBUTE : _testAttribute,
        TARGET_STYLE_CLASS : _targetStyleClass,
        APPLY_STYLE_CLASS : _applyStyleClass,

        on : function(){
            this.active = true;
            this.enforce();
        },

        off : function(){
            this.clear();
            this.active = false;
            this.unenforce();
        },

        clear : function(){
            this.filterHash = {};
        },

        add_key : function(_key){
        // messages with matching metadata will be displayed
            this.filterHash[_key] = true;
        },

        filter_only : function(_key){
            this.clear();
            this.add_key(_key);
            this.on();
        },

        test : function(_key){
            return(this.filterHash[_key]);
        },
        render : function(_$object){
            if(this.active && ((this.test(_$object.attr(this.TEST_ATTRIBUTE)) === true) === this.testFor)){
                _$object.addClass(this.APPLY_STYLE_CLASS);
            }
            return _$object;
        },

        enforce : function(){
            var i;
        // possible performance problems
            $("#" + this.CONTAINER_ID + " ." + this.TARGET_STYLE_CLASS).addClass(this.APPLY_STYLE_CLASS);
            //first hide everything...
            for(i in this.filterHash){ 
                    // For each attribute and value in add_key, remove the hidden class
                    $("[" + this.TEST_ATTRIBUTE + "='" + i + "']").removeClass(this.APPLY_STYLE_CLASS);
            }
        },

        unenforce : function(){
            $("#" + this.CONTAINER_ID + " ." + this.TARGET_STYLE_CLASS).removeClass(this.APPLY_STYLE_CLASS);
        }
    };
}
\$\endgroup\$
2
\$\begingroup\$

The usefulness of creating a class is that you can have private variables.

Simply placing every thing in an object defeats that point. Instead you should define everything in the protected part of the function and simply expose an external API with the return.

http://codr.cc/s/9b31baa5/js

\$\endgroup\$
0

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.