3
\$\begingroup\$

I have written some code which will fetch contents from a resource which is actually stored in a tree format. Since it is in a tree format, there will be a parent-child relation and hence recursion.

I am facing lot of performance issue as tree is growing in size this particular piece of code takes up-to 25 sec which is very bad. This piece of code will basically read data stored in file system (just for example) each content has certain set of property which it has to read.

import java.util.List; public class Links { private String nodeName; private List<Links> children; public List<Links> getChildren() { return children; } public void setChildren( List<Links> children ) { this.children = children; } public String getNodeName(){ return nodeName; } public void setNodeName( String nodeName ){ this.nodeName = nodeName; } } package menu; import java.util.ArrayList; import java.util.Iterator; import java.util.List; public class Utility { /* * IN this class NavModel,CModel,CNode are some dummy classes which help us read contents form some resources which * are stored in dummy format as Example of tree format stored data below is the example tree child1 child2 child3 * child3-1 child:q child:r child:a child3-2 child4 */ private static void populateLinks( NavModel navModel, ContentModel metaDataModel, Object objectNode, Links parent, boolean specialLinks ) throws IOException{ try{ List<Links> childLinks = new ArrayList<Links>(); // below code gets all the childrens of parent Iterator it = navModel.getChildren( objectNode ); while( it.hasNext() ){ NavNode node = (NavNode) it.next(); ContentNode contentNode = node.getContentNode(); Links links = new Links(); MetaData data = metaDataModel.getMetaData( contentNode ); Map<String, String> paramValues = new HashMap<String, String>(); // this particular piece of // code gets all the properties of content iterates and stores it in data structure Iterator metaDataIterator = data.getNames().iterator(); while( metaDataIterator.hasNext() ){ String pagePropertyKey = metaDataIterator.next().toString(); String pagePropertyValue = (String) data .getValue( pagePropertyKey ); if( pagePropertyKey.equalsIgnoreCase( "LINK_TYPE" ) ){ links.setLinkType( pagePropertyValue ); } else if( pagePropertyKey.equalsIgnoreCase( "TCDID" ) ){ links.setTcmIdMap( pagePropertyValue ); } else{ paramValues.put( pagePropertyKey, pagePropertyValue ); } } links.setParamValues( paramValues ); if( specialLinks ){ links.setNodeName( links.getParamValues().get( "APPLICATIONNAME" ) ); } else{ links.setNodeName( contentNode.getTitle( new Locale( "en_US" ) ) ); } links.setDisplayName( setAppDispName( links.getNodeName() ) ); childLinks.add( links ); if( navModel.hasChildren( node ) ){ // This is where recursion happens populateLinks( navModel, metaDataModel, node, links, specialLinks ); } } parent.setChildren( childLinks ); } catch( Exception e ){ } } // THis is the method which calls the recursion function public static Links setupLinks( String categoryLinkName, String name ){ Links categoryLinks = null; CModel contentModel = new CModel(); NavModel navModel = new NavModel(); categoryLinks = Utility.createCategoryLinks( categoryLinkName ); Object objectNode = contentModel.getLocator().findByUniqueName( name ); if( objectNode != null ){ if( navModel.hasChildren( objectNode ) ){ populateLinks( navModel, objectNode, categoryLinks ); } } } private static Object setAppDispName( String nodeName ){ // fetch value from db return null; } } 

I know code is not complete and may be in appropriate to review but I can't paste the whole of my code hence I hope you understand and review there is recursion and iteration any way I can better write this piece of code.

\$\endgroup\$
2
  • \$\begingroup\$:I think, this must belong to the StackOverflow\$\endgroup\$
    – Saurabh
    CommentedDec 28, 2012 at 14:54
  • \$\begingroup\$@Saurabh: No, improvements to working code fall under code review's domain\$\endgroup\$
    – seand
    CommentedDec 28, 2012 at 15:58

1 Answer 1

1
\$\begingroup\$
Iterator it = navModel.getChildren( objectNode ); while( it.hasNext() ){ NavNode node = (NavNode) it.next(); 

java.util.Iterator has an extending class, which if you use, you don't have to manally cast back after calling .next(), as follows:

Iterator<NavNode> it = navModel.getChildren( objectNode ); while( it.hasNext() ){ NavNode node = it.next(); 

Furthermore, if you are using Java 7 or above, you no longer have to specify the extending type in the assignment if you use the "diamond operand", as in these lines:

Map<String, String> paramValues = new HashMap<>();

List<Links> childLinks = new ArrayList<>();

Additionally, you are calling .equalsIgnoreCase() (See java.lang.Stringsource code) multiple times in an if-else statement, which internally calls .toUpperCase() on the String, which can happen twice in a worst case scenario of your code, therefore I believe this would be more efficient:

pagePropertyKey = pagePropertyKey.toUpperCase(); switch(pagePropertyKey) { case "LINK_TYPE": links.setLinkType( pagePropertyValue ); break; case "TCDID": links.setTcmIdMap( pagePropertyValue ); break; default: paramValues.put( pagePropertyKey, pagePropertyValue ); break; } 

As then it only has to call .toUpperCase() once and then equals() alone which is more efficient.

\$\endgroup\$
1
  • \$\begingroup\$thank you, however is there any other way i can optimize this code\$\endgroup\$
    – VIckyb
    CommentedDec 31, 2012 at 10:11

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.