How to make use of something like Factory in jquery/javascript

Question

I want to use some good coding standards for the current project. The overall outcome is achievable now (in my project), but i want help regarding following a better approach.

I'll try my best to explain what I am doing.

I have a xml Response for System and Product information (The xml is too big to accomodate here 1MB approx)

I have two panes left Pane (an accordion listing what is to be clicked) right Pane (JQUERY UI TABS inside which is JPANEL accomodating JQGRID inside it)

When the left Pane any item is clicked 3 params are sent, id of the anchor tag, Service Name & an alias (which decides what tab content should be) to function infa9Services(id,sname,alias){...}

<a id="infa9Services"\>DataIntegrationService</a> //this is the Service Name

alias is coming from a map, which maps service Name to a alias DIS (in one case)
The Right Pane contains a <div id="detailTable"></div> which gets populated from infa9Services function.

Content of infa9Services function

var tabs = [ "Service", "Property", "Additional Info" ];    
//Above array creates the Jquery Tab  heading

var lis="";
var divs="";
var panels=[];
var tpanels="";
var adtrs="";

var service=[];
var property=[];
var aDoctor=[];
var homeFileList=[];
var ssaprlist=[];
var refData=[];

fetching data from xml response xml

$(xml).find('product').each(function(){ 
    $(this).children('DomainMeta').each(function(){ 
        $(this).children('Service').each(function(){ 
            if(sname==$(this).attr('name')) //There are more than one service in the xml, so checking which Service name was clicked, in this case DataIntegrationService
            {
                $(this).children('OptionGroup').each(function(){ 

                    if($(this).attr('name').toLowerCase().indexOf("service") >= 0)
                    {
                        $(this).children('Option').each(function(){ 
                            var srow={};
                            srow.name=$(this).attr('name');
                            srow.value=$(this).attr('value');
                            service.push(srow);
                        });

                    }
                    else
                    {
                        $(this).children('Option').each(function(){ 
                            var prow={};
                            prow.name=$(this).attr('name');
                            prow.value=$(this).attr('value');
                            property.push(prow);
                        });
                    }
                });
            }
        });
    });

Now here I am checking using if else what alias is it, based on which i'll fetch data

    if(alias==="PCIS")  
    {
        $(this).children('addressDoctor').each(function(){ 
            $(this).children('addressDoctor_info').each(function(){ 

                //adding xml values to aDoctor array
            });
        });

        $(this).children('homefilelist').each(function(){ 
            $(this).children('homefilelist_info').each(function(){ 

                //adding xml values to homeFileList array
            });
        });
    }
    else if(alias==="PCRS")
    {
        $(this).children('homefilelist').each(function(){ 
            $(this).children('homefilelist_info').each(function(){ 

                //adding xml values to homeFileList array
            });
        });
    }
    else if(alias==="DIS")
    {
        $(this).children('addressDoctorEngVerDIS').each(function(){ 
            adtrs='<tr width="100%">'+
                '<th class="thPanel" align="left" valign="top" width="40%">'+$(this).children('addressDoctorEngVer_info').attr('name')+'</th>'+
                '<td align="left" width="60%">'+$(this).children('addressDoctorEngVer_info').attr('value')+'</td>'+
            '</tr>';
        });
    }
    else if(alias==="CMS")
    {
        $(this).children('referenceDataLocation').each(function(){ 
            $(this).children('referenceDataLocation_info').each(function(){
                //adding xml values to refData array

            });
        });

        $(this).children('ssaprDirList').each(function(){ 
            $(this).children('ssaprDirList_info').each(function(){
                //adding xml values to  ssaprlist array
            });
        });
    }

});

dynamically adding li 's (for UI tabs)

$.each(tabs, function(i, item) {
    lis+='<li><a href="#tabs-'+i+'">'+item+'</a></li>';
});

dynamically adding div's with class="class" (for JPanel) is there a better way here?

panels[0]='<div title="General Information" class="class">'+
            '<div class="jqUIDiv">'+
                '<table id="tblService" width="100%"></table>'+
                '<div id="ServiceGridpager"></div>'+
            '</div>'+
        '</div>';

panels[1]='<div title="General Information" class="class">'+
            '<div class="jqUIDiv">'+
                '<table id="tblProperty" width="100%"></table>'+
                '<div id="PropertyGridpager"></div>'+
            '</div>'+
        '</div>';

Here again I am making use of if else what alias is it, based on which i'll populate data from the vars which were fetched before

if(alias==="PCIS")
{
    panels[2]='<div title="Address Doctor" class="class" >'+
                    '<table class="jpanelTable">'+
                        '<tbody>'+adtrs+'</tbody>'+
                    '</table>'+
                '</div>';

    panels[2]+='<div title="Address Doctor Configuration" class="class">'+
                    '<div class="jqUIDiv">'+
                        '<table id="tblAdditional" width="100%"></table>'+
                        '<div id="AdditionalGridpager"></div>'+
                    '</div>'+
                '</div>';

    panels[2]+='<div title="Home File List" class="class">'+
                '<div class="jqUIDiv">'+
                    '<table id="tblHomeFileList" width="100%"></table>'+
                    '<div id="HomeFileListGridpager"></div>'+
                '</div>'+
            '</div>';
}
else if(alias==="PCRS")
{
    panels[2]='<div title="Home File List" class="class">'+
                    '<div class="jqUIDiv">'+
                        '<table id="tblHomeFileList" width="100%"></table>'+
                        '<div id="HomeFileListGridpager"></div>'+
                    '</div>'+
                '</div>';
}
else if(alias==="DIS")
{
    panels[2]='<div title="Address Doctor" class="class" >'+
                '<table class="jpanelTable">'+
                    '<tbody>'+adtrs+'</tbody>'+
                '</table>'+
            '</div>';
}
else if(alias==="CMS")
{
    panels[2]='<div title="Reference Data Location" class="class">'+
                    '<div class="jqUIDiv">'+
                        '<table id="tblRefData" width="100%"></table>'+
                        '<div id="RefDataGridpager"></div>'+
                    '</div>'+
                '</div>';

    panels[2]+='<div title="SSAPR" class="class">'+
                '<div class="jqUIDiv">'+
                    '<table id="tblSSAPR" width="100%"></table>'+
                    '<div id="SSAPRGridpager"></div>'+
                '</div>'+
            '</div>';
}
else
{   
    panels[2]='<div title="Additional Information" class="class" >'+
                '<table class="jpanelTable">'+
                    '<tbody>'+
                        '<tr width="100%">'+
                            '<td align="left" style="word-break:break-all;">'+NA+'</td>'+
                        '</tr>'+
                    '</tbody>'+
                '</table>'+
            '</div>';
}

Note: panel[0],panel1,panel[2] are contents of the 3 tabs

dynamically adding div 's (content for Jquery UI Tab)

$.each(panels, function(i, item) {
    divs+='<div id="tabs-'+i+'">'+item+'</div>';
});

adding the whole thing to detailTable (Right Pane)

$('#detailTable').empty();
$('#detailTable').html('<div class="titleBlue"><a href="#" class="navA">Configuration</a>&gt;<a href="#" class="navA">'+productname+'</a>&gt;'+sname+' ('+alias+')</div>');

$("<div id='loadingDiv'><table id='detailTable' width='100%'><tbody><tr><td align='center'>Please wait</td></tr><tr><td align='center'><img src='/csm/view/include/images/loading.gif' alt='Loading'/></td></tr></tbody></table></div>").appendTo('#detailTable');
$('<div id="tabs">'+'<ul>'+lis+'</ul>'+divs+'</div>').appendTo('#detailTable').delay(10).queue(function(){
    $('.class').jPanel({
        'effect'    : 'fade',
        'speed'     : 'medium',
        'easing'    : 'swing'
    });

});

More.. JQGrids to populate tblService, tblSSAPR, tblRefData etc tables (not included here)

How can I make use of something like Factory or something more Generic in such a case? If suppose there are a lot of alias to be checked it will be very cumbersome to do it the way I am doing it currently. Any help would be appreciated. Thanks

Screen shot enter image description here

Update

My XML content

<?xml-stylesheet type="text/xsl" href="csmclientinfa9.xsl"?>
<csmclient product="infa9" date="12/16/11 12:10 PM" version="1.0">
    <system>
    </system>
    <product>
        <DomainMeta>

            <Service type="PCIntegrationService" name="TestPCInteg" version="" licenseName="lic_PCIS">    //THIS IS PCIS SERVICE
                <ServiceProcess>
                    <Node name="N_1a63931"></Node>
                    <PreStartCommand> null</PreStartCommand>
                    <PostStartCommand> null</PostStartCommand>
                    <JvmOptions> </JvmOptions> 
                    <OptionGroup name="IntegrationServiceProcessOptions">
                        <Option  name="Codepage_Id" value="4" ></Option>
                        <Option  name="$PMSourceFileDir" value="$PMRootDir/SrcFiles" ></Option>
                    </OptionGroup>
                </ServiceProcess>
                <OptionGroup name="IntegrationServiceOptions">
                    <Option  name="ServiceResilienceTimeout" value="180" ></Option>
                </OptionGroup>
            </Service>

            <Service type="DataIntegrationService" name="TestDIS" version="" licenseName="lic_DIS">    //THIS IS DIS SERVICE
                <ServiceProcess>
                    <Node name="N_1163931"></Node>
                    <PreStartCommand> null</PreStartCommand>
                    <PostStartCommand> null</PostStartCommand>
                    <JvmOptions> -Dfile.encoding=UTF-8 -server -Xms256M -Xmx512M -XX:GCTimeRatio=19 -XX:+UseConcMarkSweepGC -XX:+UseParNewGC -XX:ParallelGCThreads=4 -XX:NewRatio=2</JvmOptions> 
                    <OptionGroup name="SQLServiceOptions">
                        <Option  name="MaxConcurrentConnections" value="100" ></Option>
                        <Option  name="MaxPlanCacheSize" value="100" ></Option>
                    </OptionGroup>
                    <OptionGroup name="WebServiceOptions">
                        <Option  name="MaxPlanCacheSize" value="100" ></Option>
                    </OptionGroup>
                </ServiceProcess>
                <OptionGroup name="SQLServiceOptions">
                    <Option  name="ConnectionCleanupPeriod" value="30000" ></Option>
                    <Option  name="DTMKeepAliveTime" value="0" ></Option>
                </OptionGroup>
                <OptionGroup name="CoreServiceOptions">
                    <Option  name="SafeModeLevel" value="0" ></Option>
                </OptionGroup>
            </Service>

        </DomainMeta>

        <homefilelist>
            <homefilelist_info permission='-rwxr-xr-x' hardlink='1' owner='idp' group='support' fsize='61597' date='22 Mar 2011' filename='libpmwspsvrcmn.so' />
            <homefilelist_info permission='-rwxr-xr-x' hardlink='1' owner='idp' group='support' fsize='21778' date='22 Mar 2011' filename='libpmorablk.so' />
        </homefilelist>

        <addressDoctor filename='AD100.cfg'>
            <addressDoctor_info name='MemoryUsage' value='1048' />
            <addressDoctor_info name='PreloadingMethod' value='MAP' />
        </addressDoctor>

        <addressDoctorEngVerPCIS>
            <addressDoctorEngVer_info name='EngineVersion' value='5.2.5' />
        </addressDoctorEngVerPCIS>

        <addressDoctorEngVerDIS>
            <addressDoctorEngVer_info name='EngineVersion' value='5.2.6' />
        </addressDoctorEngVerDIS>

        <referenceDataLocation location='/home/idp/av/default'>
            <referenceDataLocation_info permission='-rw-r--r--' hardlink='1' owner='idp' group='support' fsize='196' month='Aug' date='2' time='11:06' filename='copyright.txt'/>    
        </referenceDataLocation>

        <ssaprDirList>
            <ssaprDirList_info permission='-rw-r--r--' hardlink='1' owner='idp' group='support' fsize='2737558' month='Oct' date='14' time='2010' filename='turkey.ysp' />
        </ssaprDirList>
    </product>
</csmclient>

More Updates

        var aDoctor=[];
    var adtrs="";
    var homeFileList=[];

    if(alias==="PCIS")
    {
        $(this).children('addressDoctorEngVerPCIS').each(function(){ 
            adtrs='<tr width="100%">'+
                '<th class="thPanel" align="left" valign="top" width="40%">'+$(this).children('addressDoctorEngVer_info').attr('name')+'</th>'+
                '<td align="left" width="60%">'+$(this).children('addressDoctorEngVer_info').attr('value')+'</td>'+
            '</tr>';
        });

        $(this).children('addressDoctor').each(function(){ 
            $(this).children('addressDoctor_info').each(function(){ 
                var adevrow={};
                adevrow.name=$(this).attr('name');
                adevrow.value=$(this).attr('value');
                aDoctor.push(adevrow);
            });
        });

        $(this).children('homefilelist').each(function(){ 
            $(this).children('homefilelist_info').each(function(){ 
                var row={};
                isPresent=true;
                row.permission=$(this).attr('permission');
                row.hardlink=$(this).attr('hardlink');
                row.owner=$(this).attr('owner');
                row.group=$(this).attr('group');
                row.fsize=$(this).attr('fsize');
                row.date=$(this).attr('date');

                row.filename=$(this).attr('filename');
                homeFileList.push(row);
            });
        });
    }

    /*moving on for HTML construction, panels[..]
    panels[0] is for 1st jquery ui tab
    panels[1] is for 2nd jquery ui  tab
    panels[2] is for 3rd jquery ui  tab*/

    /*I am also using jqGrid, so I am giving one example for PCIS, how I am using aDoctor array */

    if(alias==="PCIS")
    {
        //Additional Information
        if(aDoctor.length>0)
        {
            jQuery("#tblAdditional").jqGrid({
                datatype: "local",    
                data: aDoctor,
                colNames:['Name','Value'],
                colModel:[
                    {name:'name',index:'name', align:"left"},   
                    {name:'value',index:'value', align:"left"}
                ],

                pager : '#AdditionalGridpager',
                rowNum:10,
                rowList:[10,50,100],
                scrollOffset:0,
                height: 'auto',
                autowidth:true,
                viewrecords: true,
                gridview: true
            });

            jQuery("#tblAdditional").setGridParam({rowNum:10}).trigger("reloadGrid");
            jQuery("#tblAdditional").jqGrid('filterToolbar',{stringResult: true, searchOnEnter: false, defaultSearch: 'cn'});
        }
        else //i am displaying none in the table
    }

    //So now my panel is complete for PCIS, if you refer my question, all code is available :) I know it's really big, but this is my 1st project and I want it to be better than this :) Thanks

This question has too much code. I'm having a hard time understanding what is the real problem. — hugomg
– hugomg, Commented Dec 12, 2011 at 12:44
@missingno: Hi, i know.. but if you read the text's (apart from the code) and then refer the code, you'll understand it. I tried my best to describe my problem — AabinGunz
– AabinGunz, Commented Dec 12, 2011 at 12:47
@landons it looks like the code is already written but he wants it to be refactored. — Bot
– Bot, Commented Dec 13, 2011 at 16:29
You've already got a bounty out on this question, but I think "how do I refactor my code" type questions are generally a better fit on the Code Review site. — Merlyn Morgan-Graham
– Merlyn Morgan-Graham, Commented Dec 16, 2011 at 10:44

Merlyn Morgan-Graham · Accepted Answer · 2011-12-16 11:31:52Z

4

+50

The Factory design pattern must have a switch (if/else block or associative array) under the hood anyhow. You won't ever shorten code by using it. The only benefit you'll get here is that you might hide away or separate some of that long code. Returning a function from another function, or returning an array where one of the properties is a function (set in the factory) is about as close as you can get to this pattern in JavaScript. The code sample below show something very much like a factory that is just as good in the JavaScript case, and might be slightly less confusing.

I'm not sure what you mean by Generic. If you're referring to "generics", ala Java or C#, then it won't help you in this scenario. Generics in those languages help you get around static typing and avoid duplicating classes. You're not using classes at all here.

If you're simply trying to avoid repeating yourself, you should look for a functional refactor here. See what repeats, and what doesn't. Figure out how you can loop or map to the stuff that doesn't repeat, and write the code that does repeat only once.

For example, I'd use an associative array to map strings to functions for your big if/else block, and I'd extract that child-looping code into a more generialized function:

function forAll(parentName, childName, action) {
    $(this).children(parentName).each(function(){ 
        $(this).children(childName).each(function(){ 
            action();
        });
    });
}

var aliasToHandlerMap =
{
    "PCIS": function() {
        forAll('addressDoctor', 'addressDoctor_info', function() {
            //adding xml values to aDoctor array
        });
        forAll('homefilelist', 'homefilelist_info', function() {
            //adding xml values to homeFileList array
        });
    },
    "PCRS": function() {
        forAll('homefilelist', 'homefilelist_info', function() {
            //adding xml values to homeFileList array
        });
    },
    "DIS": function() {
        $(this).children('addressDoctorEngVerDIS').each(function(){ 
            adtrs=
            '<tr width="100%">'+
                '<th class="thPanel" align="left" valign="top" width="40%">'+
                    $(this).children('addressDoctorEngVer_info').attr('name')+'</th>'+
                '<td align="left" width="60%">'+
                    $(this).children('addressDoctorEngVer_info').attr('value')+'</td>'+
            '</tr>';
        });
    },
    "CMS": function() {
        forAll('referenceDataLocation', 'referenceDataLocation_info', function() {
            //adding xml values to refData array
        });

        forAll('ssaprDirList', 'ssaprDirList_info', function() {
            //adding xml values to  ssaprlist array
        });
    }
};

// Execute the appropriate handler for the particular alias
var handler = aliasToHandlerMap[alias];
handler();

In this case I'd also recommend you name each of those handlers, and build the map using named functions. I just wanted to show you visually how much code compression you get from this.

You should be able to do similar things for your long strings of HTML that you're building. If they follow a general pattern, make a function for them, and extract the parts that change into parameters that you pass in.

And you generally shouldn't be building styles into your elements (e.g. align="left" valign="top" width="40%"). You should use style sheets that apply to those specific items, applying only CSS classes when necessary, so that you to specify that in only one place. That should shorten a lot of the element creation code, and might let you use jQuery DOM creation to avoid messy string concatenation code.

All refactoring of long code boils down to an application of the same principle - find out what repeats, and what doesn't repeat. Extract the part that repeat, and build an iterable or indexable structure out of the part that doesn't repeat.

To start learning how to do generalized refactoring in languages using a Functional Programming style, I suggest you watch the SICP lecture videos. They use LISP, but teach you it while you're watching the videos, and the principles apply to every language out there. That language lends itself extremely well to this type of refactoring, which is why he uses it.

You could also look into CoffeeScript, which allows you to take java code and shrink it into a more compact syntax :) It compiles back into pretty-printed JavaScript, so you don't have to worry about server impact, installing code on client boxes, etc.

Also, check out a few of these links for more reading on general code refactoring:

http://en.wikipedia.org/wiki/Code_refactoring
http://martinfowler.com/refactoring/ (he wrote a book)

edited Dec 16, 2011 at 11:31

answered Dec 16, 2011 at 10:33

Merlyn Morgan-Graham

59.4k16 gold badges131 silver badges189 bronze badges

Sign up to request clarification or add additional context in comments.

12 Comments

AabinGunz Over a year ago

Looks good :) thanks. My db server is currently on maintenance i'll work on re factoring my code from your sources. Very nice explanation

AabinGunz Over a year ago

I am not able to understand forAll function. In aliasToHandlerMap >PCIS forAll is getting called with some parameters, which is a function itself. And the actual called function forAll iterates over parentName & childName which again calls action.. So I am unable to understand what is the relation here in the actual forAll function calling action() and the function which calls forAll() with params, which again has a function block having //adding xml values to aDoctor array

Merlyn Morgan-Graham Over a year ago

@AbhishekSimon: Look at the original code and compare it side by side with both the reduction, and the contents of the forAll function. No less code gets executed, but you don't have to repeatedly write $(this).children('homefilelist').each(function(){ $(this).children('homefilelist_info').each(function(){ - you instead just write forAll('addressDoctor', 'addressDoctor_info', function() {. It is a minor reduction of duplication, but it is a simple example of the sort of pattern you should be looking for in all code duplication.

Merlyn Morgan-Graham Over a year ago

@AbhishekSimon: See this fiddle for a contrast between something that works like the original code, and something that works like the updated code - jsfiddle.net/r744j - You can see the exact same work is done, but the pattern is abstracted so you can focus on the implementation and differences between each code block a bit, since the plumbing isn't getting in the way. If you provide some sample XML building code I could also show you how to eliminate some of the duplication inside those blocks.

AabinGunz Over a year ago

Thanks for the jsfiddle link.. i am looking at it right now, also I put a small (relevant) portions of my xml content, which contains 2 services PCIS & DIS along with it, it also contains homefilelist, addressDoctor, referenceDataLocation, addressDoctorEngVerPCIS, addressDoctorEngVerDIS & ssaprDirList nodes to give you a picture of what information gets generated under which service.

|

Bot · Accepted Answer · 2011-12-13 16:33:41Z

0

I think you are asking for a better way to make your code without having to hard code the html? If that is the case you could use jQuery.template() http://api.jquery.com/jQuery.template/. It's in beta but it should help you by not having to hardcode your html into the javascript.

For my projects we do our templating in PHP and just have jquery do ajax calls if the data or templates need to change.

If this is not what you are looking for can you be more descriptive with your question?

answered Dec 13, 2011 at 16:33

Bot

11.9k11 gold badges78 silver badges134 bronze badges

1 Comment

AabinGunz Over a year ago

Thanks for the information, it might be helpful when it gets released, right now i don't want any such if conditions clubing up my space eg: if(alias==="PCIS") I want a more uniform way where depending upon the alias it should decide what content should it have.

Deepu · Accepted Answer · 2012-03-20 06:44:19Z

0

If you are looking at refactoring further you can use the same jPanel to buld tabs using just a simple div markup like you did for accordion just replace your jPanel js file with the latest 1.4 version that will save some markup junk for you and also you have too much of html as strings try using a javascript template engine here

answered Mar 20, 2012 at 6:44

Deepu

1,3011 gold badge11 silver badges26 bronze badges

Collectives™ on Stack Overflow

How to make use of something like Factory in jquery/javascript

3 Answers 3

12 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

12 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related