Exam 1 Comments

Specifying Procedures

public class StringTable 
{
    // overview: StringTable is a set of <String, double> entries,
    //    where the String values are unique keys.  A typical StringTable
    //    is {<s0: d0>, <s1: d1>, ... }.
    //

    ...
}

public boolean deleteName (String name)
   // REQUIRES: The parameter name is not null.
   // EFFECTS: Goes through the entries in the elements Vector, and removes
   //     the one whose key matches name.
   

There are dozens of problems with Muddle's specification! The most important are:

• It is written in terms of the concrete representation, not the abstract type. Specifications should not be tied to particular implementations.
• It is written in an operational (how) way ("goes through"). Specifications should be declarative (explain what it does, not how).
• It is missing a modifies clause. Based on its name and effects clause, it should modify the this object.
• Its postcondition does not explain what happens in all situations: what if the name is not in this? what is it is in the elements Vector more than once? (Of course, if the specification were written in terms of the abstraction, this wouldn't be a problem.)
• The method header has a boolean return type, but the effects clause does not explain what this type means.

public boolean deleteName (String name)
   // REQUIRES: The parameter name is not null.
   // MODIFIES: this
   // EFFECTS: If name matches a key in this, removes the entry
   //    associated with that key and returns false.  Otherwise,
   //    does not modify this and returns true.

We could also describe its behavior using our abstraction notation:

        e.g., If this_pre = { <s0: d0>, <s1: d1>} and
   s0 equals name, result == true and this_post == {
   <s1: d1>}.  If name does not equal s0 or
   s1, then result == true and this_post == this_pre.

Data Abstractions

The most obvious problem is there is no method to get the weight associated with an edge. Without this, the abstraction isn't adequate — we would be better using a normal Graph without edge weights. So, we should add a method getEdgeWeight:

   public int getEdgeWeight (String fnmode, String tnode)
      throws NoNodeException
      // EFFECTS: If fnode or tnode is not the name of a node in
      //   this.nodes, throws NoNodeException.  Otherwise, if
      //   there is an edge from fnode to tnode in the graph,
      //   returns the weight of that edge.  If there is no 
      //   edge, returns 0.

Note that this.nodes in our specification means the abstract nodes set used in the overview, not anything to do with our rep (which happens to use nodes also). Perhaps, our specification should throw an exception if the node does not exist instead of returning 0.

To be useful, the abstraction should have some more observers: getNeighbors (returns an iterator that yields the neighbors of a node), and perhaps nodes (returns an iterator that yields all the nodes in the graph).

For some purposes, it would be more useful to be able to have zero-weight edges (but there are plenty of applications that can use the abstraction with this restriction).

To be immutable, the type must not provide any mutators. So, we would need to remove the addNode and addEdge methods. If we just removed them, we would have an immutable datatype, but it wouldn't be useful. All we could do is create an empty graph and call the observers on it. So, we need to either:

• Provide a constructor that can create a particular graph (for example, by reading it from a file).
• Or, provide producer versions of the addNode and addEdge methods. Instead of modifying the this object, these methods would create and return a new graph (with the additional node or edge).

Possible advantages of making WDGraph immutable are that it makes it easier to reason about code (we don't need to worry about aliasing). A disadvantage is that implementing addNode and addEdge as producers instead of mutators will make them more complicated and require extra storage (creating all those new graphs) and computation (copying).

   // Rep:
   String nodes[]; 
   int edges[][];

   // Abstraction Function:

   // AF (c) = < nodes, edges > where
   //          nodes = { c.nodes[i] | 0 < i <= c.nodes.length }
   //          edges = { < c.nodes[fi], c.nodes[ti], c.edges[fi][ti] > |
   //                    0 < fi <= c.nodes.length,
   //                    0 < ti <= c.nodes.length 
   //                    where c.edges[fi][ti] != 0
   //
   // For example, the representation below would represent the flight
   // graph described in the overview:
   //    
   //     c.nodes = [ "Charlottesville", "Dulles", "Richmond" ]
   //     c.edges = [ [   0, 119, 89],
   //                 [ 139,   0, 0 ],
   //                 [   0,  79, 0 ]
   //

The hasEdge method is implemented as:

   private int lookupNode (String node) throws NoNodeException
      // EFFECTS: If node is a node in this.nodes, returns the
      //    index such that this.nodes[result].equals (node).
      //    Otherwise, throws NoNodeException.

      //   (Note that lookupNode is a private method.  Hence, it is okay that we
      //    have specified it using the concrete representation instead of in
      //    terms of the abstraction.)

   public boolean hasEdge (String fnode, String tnode) 
      throws NoNodeException {
      return (edges[lookupNode (fnode)][lookupNode (tnode)] != 0);
   }

The best way to think about this is to look at the code for hasEdge and determine what needs to be true for it to work:

      return (edges[lookupNode (fnode)][lookupNode (tnode)] != 0);

We are indexing the edges array, so it must not be null:

//@invariant edges != null

We are calling lookupNode. It has no require clause, though, so nothing has to be true for this to be okay.

We are indexing edges[lookupNode (fnode)]. For this to be in bounds, we need to know that the result of lookupNode (fnode) is between 0 and the length of edges - 1. The effects clause for lookupNode tells us that its result is a valid index of the nodes array, hence it is between 0 and nodes.length - 1. From this, we can infer that the edges array must have at least as many elements as the nodes array:

//@invariant edges.length >= nodes.length

Next, we index in the second array: edges[lookupNode (fnode)][lookupNode (tnode)]. So, edges[lookupNode (fnode)] must not be null. Further, the length of each edges[i] array must be >= nodes.length:

// invariant forall 0 <= i < nodes.length
//     edges[i] != null
//     edges[i].length >= nodes.length

That's all we can definitively say about the rep invariant from just looking at what is necessary for hasEdge. It is likely that lookupNode will require the nodes array contain no duplicates.

Testing

The important thing to remember about black box test cases is that they are based on the specification, not a particular implementation. We should test all paths through the specification of addNode:

      // EFFECTS: If node is the name of a node in this.nodes,
      //    throws the DuplicateNode exception.  Otherwise, 
      //    adds a node named name to this:
      //        this_post = < this_pre.nodes U { node }, this_pre.edges >

There are two obvious paths:

• The node already exists in the graph.
• The node does not already exist in the graph.

Reasonable test cases would be:

        WDGraph g = new WDGraph ();
	g.addNode ("Charlottesville");
	g.addNode ("Dulles");
	g.addNode ("Dulles"); // should throw DuplicateNodeException

We should also test boundary or special cases: the node is null, and the graph is empty.

7. (6.9 / 10) Suppose we implement addNode as below. What additional glass box test cases (if any) are necessary?

      public void addNode (String node) throws DuplicateNodeException {
         try {
	    lookupNode (node);
	    throw new DuplicateNodeException (node);
	 } catch (NoNodeException e) {
	    String [] oldnodes = nodes;
	    nodes = new String [oldnodes.length + 1];

	    for (int i = 0; i < oldnodes.length; i++) {
	       nodes[i] = oldnodes[i]; 
	    }
	    nodes[oldnodes.length] = node;
	}
     }	

The glass box test cases should excercise paths through the implmentation. One path is when lookupNode does not throw a NoNodeException. Our black box test cases already cover this when we add a duplicate node. Another path is when lookupNode throws an exception and we execute the catch block. This block has essentially infinitely many paths through the loop — we can't test them all, but we should test it for 0, 1 and several iterations. The first addNode black box test cases tests it for 0, the second tests it for 1. We need some additional tests to test it for several:

        WDGraph g = new WDGraph ();
	g.addNode ("Charlottesville");
	g.addNode ("Dulles");
	g.addNode ("Richmond");
	g.addNode ("Reagan");

The really serious flaw is that it does not preserve the rep invariant. Among other things, our rep invariant requires: edges.length >= nodes.length. The implementation of addNode increases nodes.length by 1. We need to increase edges.length by 1 also to preserve the rep invariant. This makes sense — addNode should extend each edges array by 1 so that edges involving the new node can be stored. Note that our addNode tests would not reveal this problem. It only becomes apparent when we call a method like hasEdge that depends on the edges array satisfying the rep invariant.

Another possible flaw is the code does not address what happens if node is null. The lookupNode specification implies that nodes in the graph cannot be null, but does not have a requires that node is not null. As specified, it would throw the NoNodeException if node is null (but a better lookupNode spec would make this much more clear). Hence, if addNode is passed a null node, it would add it to the array, which would violate the rep invariant. The best solution to this is probably to just add requires node != null to the specification for addNode.