2
\$\begingroup\$

I am trying to get ordered components of an interface properly without having a local variable orderedComponents outside, and here is my current code.

Reason is because orderedComponents will be in Interface.java and getChildren will be in componentDefinitions.java and I want a more controllable code.

I was also thinking of changing from List to single dimension array and ill soon remove the static ones I separate them.

public static List<Integer> orderedComponents = new ArrayList<Integer>();

/**
 * returns the components ordered
 * 
 * @param interfaceId
 * @return The components of an interface sorted
 */
public static List<Integer> getOrderedComponents() {

    orderedComponents.clear();

    for (int i = 0; i < inter.getComponents().length; i++) {

        // if all comps are added, stop the loop
        if (orderedComponents.size() == inter.getComponents().length)
            break;

        if (orderedComponents.isEmpty() && inter.getComponent(i).hasParent()
                && !orderedComponents.contains(inter.getComponent(i).getParentId()))
            continue;

        // do the magic
        getAllChildren(i);

    }
    return orderedComponents;
}

public static void getAllChildren(int componentId) {

    // get the current component definitions
    ComponentDefinitions component = inter.getComponent(componentId);

    // TODO first parent of the parents
    // if component has parent and it is not added to the list, add it
    if (component.hasParent() && !orderedComponents.contains(component.getParentId())) {
        orderedComponents.add(component.getParentId());
    }

    // if component is not added to the list, add it
    if (!orderedComponents.contains(component.componentId)) {
        orderedComponents.add(component.componentId);
    }

    // loop through all childs of the current components and re-add their childs
    for (int j = 0; j < inter.getComponent(componentId).getChilds().size(); j++) {
        ComponentDefinitions child = inter.getComponent(componentId).getChilds().get(j);
        getAllChildren(child.componentId);
    }
}

Please let me know what improvements could be done here and what I am doing wrong or is advisable to be changed.

\$\endgroup\$
0
\$\begingroup\$

If I understood correctly, you have to traverse a list of components where each element has a hierarchy-like structure?

Try the visitor pattern https://www.tutorialspoint.com/design_pattern/visitor_pattern.htm it is a very clean solution of what you're trying to do without any static methods.

And for traversing the children of each component, I think you're trying to do a pseudo-BFS, but it's a bit confusing. BFS is very tricky using recursion. The iterative version kf BFS should be something similar to this https://stackoverflow.com/questions/16380026/implementing-bfs-in-java

\$\endgroup\$
0
\$\begingroup\$

It’s not really clear what you are trying to achieve. Judging from the code smells I see, this code needs a helper method/object. Don’t be afraid of adding private functions, besides the ones defined in the public interface.

During a traversal, a collection of already visited objects needs to be maintained. The entry method should create it locally and pass it to another private method visit(componentId, visited). This way the visited collection will get automatically garbage collected, when you are finished.

From the performance point of view, it seems that you need a LinkedHashMap, not an ArrayList.

\$\endgroup\$

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.