4
\$\begingroup\$

I feel like there is a redundency between:

def _determine_nodes_with_no_predecessors(nodes:List[Node])->List[str]:
    """
    Given the list of nodes returns the identifiers of nodes that doesn't have any predecessor. 
    """
    nodes_ids_without_prdecessors = []
    for node in nodes:
        if not node.predecessors: 
            nodes_ids_without_prdecessors.append(node.id_)
    return nodes_ids_without_prdecessors


def _add_start_node(graph:Graph):

    start_node = Node.create_start_node(graph)

    graph.add_node(start_node)

    nodes_ids_without_prdecessors = _determine_nodes_with_no_predecessors(graph.nodes)
    
    # Its successors are the nodes without predecessors in this graph
    for node_id in nodes_ids_without_prdecessors: 
        start_node.add_successor(graph.nodes[node_id])
            
    # Conversly, We add the `start_node` as a predecessor to the nodes without predecessors in this graph
    for node_id in nodes_ids_without_prdecessors: 
        node = graph.nodes[node_id]
        node.add_predecessor(start_node)

And this one:

def _determine_nodes_with_no_successors(nodes:List[Node])->List[str]:
    """
    Given the list of nodes returns the identifiers of nodes that doesn't have any successor. 
    """
    nodes_ids_without_successors = []
    for node in nodes:
        if not node.successors: 
            nodes_ids_without_successors.append(node.id_)
    return nodes_ids_without_successors

def _add_end_node(graph:Graph):
    
    end_node = Node.create_end_node() #constructor of node
    
    graph.add_node(end_node)
    
    end_node = graph.nodes[end_node.id_]

    nodes_ids_without_successors = _determine_nodes_with_no_successors(graph.nodes)

    # Its predecessors are the nodes without successors in this graph
    for node_id in nodes_ids_without_successors: 
        end_node.add_predecessor(graph.nodes[node_id])
            
    # Conversly, We add the `end_node` as a predecessor to the nodes without predecessors in this graph
    for node_id in nodes_ids_without_successors: 
        node = graph.nodes[node_id]
        node.add_successor(end_node)

Note: Node is a class that have the attributes predecessors, successors and id_ and the class methods create_dummy_start_node, create_dummy_end_node, add_predecessor and add_successor. Graph is a class that have the attribute nodes and the method add node.

How to write less code in this case? Is it possible to write only two functions that do the whole stuff (instead of four functions)?

\$\endgroup\$
1
\$\begingroup\$

You can use the built-in getattr function to extract an attribute by name. Likewise the setattr function to set attributes.

This means you can pass the name of the attribute to a private function, unifying the code:

def _find_nodes_with_no(pred_succ: str, nodes: List[Node]) -> List[Node]:
    """ Find nodes with either no predecessors, or no successors. """

    if pred_succ not in ("predecessors", "successors"):
        raise ValueError("pred_succ must be either 'predecessors' or 'successors'")

    return [node for node in nodes if not getattr(node, pred_succ)]

Your add_start and add_end functions can thus be written to pass the appropriate names:

def add_start(graph: Graph):
    info = ("create_start_node", "predecessors", "successors")
    _add_outer_node(graph, info)

def add_end(graph: Graph):
    info = ("create_end_node", "successors", "predecessors")
    _add_outer_node(graph, info)

Then in _add_outer_node you can do things like:

cmeth, main_links, back_links = info
create_method = getattr(Node, cmeth)
new_node = create_method(graph)

# ...

outer_nodes = _find_nodes_with_no(main_links, graph)

One thing I do question it your "manual" connection of both the predecessor and successor links. It seems like you want those to be reflexive, so why not create an add_predecessor method that automatically creates the reciprocal link? Likewise with add_successor?

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Since there's only two cases, the strategy pattern should be sufficient. Alternatively, creating an inverted view of the graph would allow you to implement _add_end_node() by calling _add_start_node() on the inverted graph view.

class StartNodeStrategy:
    def create_node(self):
        return Node.create_start_node()
    def find_hanging(self,nodes):
        return [
            node.id_
            for node in nodes
            if not node.predecessor
        ]
    def fix_hanging(self,outer_node,inner_node):
        inner_node.add_predecessor(outer_node)
        outer_node.add_successor(inner_node)

class EndNodeStrategy:
    def create_node(self):
        return Node.create_end_node()
    def find_hanging(self,nodes):
        return [
            node.id_
            for node in nodes
            if not node.successor
        ]
    def fix_hanging(self,outer_node,inner_node):
        inner_node.add_successor(outer_node)
        outer_node.add_predecessor(inner_node)


def _add_outer_node(graph:Graph,strategy):
    
    outer_node = strategy.create_node()
    graph.add_node(outer_node)
    outer_node = graph.nodes[outer_node.id_]

    for inner_node in strategy.find_hanging(graph.nodes):
        strategy.fix_hanging(outer_node,inner_node)
| improve this answer | |
\$\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.