diva.graph: Design review, September 11th, 1998

Preliminary notes

Review called by John Reekie and Michael Shilman for the Diva Graph package. Eager to stabilize API as soon as possible.

Review Materials

http://ptolemy.eecs.berkeley.edu/~johnr/diva/api/diva/graph/package-summary.html

diva.graph.Edge
diva.graph.Graph
diva.graph.GraphEvent
diva.graph.GraphListener
diva.graph.GraphModel
diva.graph.Node
diva.graph.Raw*

http://ptolemy.eecs.berkeley.edu/~johnr/diva/api/diva/util/package-summary.html

diva.util.PropertyContainer

Participants

Review started:1.30 PM
Review ended: 3.00 PM

Author's Response

This design review seemed less productive than last time, but I think that was my fault for changing the class structure so much and not providing a clear list of changes (though it would have been quite large). Also the poor documentation on GraphModel and the lack of resolution on the event types was bad. But I have finished writing the interfaces and their implementations, and am nearly ready for a "release" of this low-level part of the package. All that remains is a little more documentation of how listeners should make use of the events, which is probably a short document in itself. Thank you all so much for your time and patience.

Identified defects

  1. Raw vs. cooked. Does the benefit of having separate graph, rawgraph, etc., justify the cost?

    Apparently not, according to the reaction. The Raw interfaces have been removed and folded into the "cooked" ones.

  2. Edge
    1. Is weight foundamental to the graph?
    2. Should there be a default value for weights?
    3. How about let the weight be a property instead of a member variable, so that the user can specify the type of the weight himself?

      Weights have been removed from the graph interface.

  3. Node
    1. Clarify the behavior of remove in Iterator.

      Done. (remove throws an exception)

    2. When calling inEdges() and outEdges(), what happens if there are no edges?

      Done. (a iterator with no elements is returned)

    3. Clarify who sets visited false and when.

      Done. (an algorithm should "setVisited(false)" before each pass.)

  4. RawGraphModel
    1. This class needs to be redocumented.

      Done.

    2. addEdge function
      1. What happens if the nodes are not in the graph?

        The nodes are connected by the edge and an event is dispatched. This is now generally discussed in the class-level documentation.

      2. If there's no concept of adding an edge to the graph, it does make sense to say addEdge. Maybe it should be named connectEdge?

        Renamed.

      3. The behavior of the method needs to be clarified.

        Done.

      4. addNode(node, graph) function
        1. Needs more comments.

          Done.

        2. What happens if the graph is not a subgraph of this model's graph?

          Same issue as the "addEdge" issue above, and same response.

    3. dispatchOff, dispatchOn functions
      1. Argument symmetry. dispatchOn takes in a graph as an argument, but dispatchOff doesn't.

      2. Why does dispatchOff turn off everything? This seems kind of arbitrary.

        I don't remember this point being raised, but doing anything more fine-grained would force additions to an already substantial interface.

    4. Need a policy for constructing graph hierarchy.

      Graph hierarchy is not officially supported in release 1.

    5. Should add a function, setDispatchEnabled.

      Done.

    6. getGraph needs to be recommented.

      Done.

    7. getRoot function
      1. Why is there not a getParent function.
      2. No one gets it.

        This method has been removed from the interface, as it is not necessary for a client to know about. Also hierarchical graph models will not be supported in this release.

    8. removeEdge function
      1. Is the name appropriate?

        Renamed to disconnectEdge().

    9. removeNode function
      1. What if the node to be removed is not in the graph?

        See above.

    10. RawGraphModel interface seems to be quite heavyweight. Is it too much work for the user to implement all these functions?

      I have eliminated a few methods by collapsing setNode/Edge/GraphProperty into setProperty(PropertyContainer).

  5. GraphEvent
    1. What's the policy for view to maintain state between edgeDisconnected and edgeConnected?

      I have documented this in GraphListener.

    2. NODE_DELETED should be NODE_REMOVED.

      Done.

  6. GraphListener
    1. Currently UserObjectContainer generates GraphEvent. Should it generate UserObjectEvent to be consistent?

      Yes it does. This is a generic capability, so UserObjectChangeEvent and UserObjectChangeListener have been added.