0

Consider the following two functions in a class called Graph. The entire source code is found here: http://www.keithschwarz.com/interesting/code/?dir=dijkstra. 

public void addNode(T node) {
        mGraph.put(node, new HashMap<T, Double>());
    }

public void addEdge(T start, T dest, double length) {
        mGraph.get(start).put(dest, length);
    }

Here, the addEdge method is blindly trusting addNode method that it has added hashmap to mGraph. Is it a common practice to trust that other methods in class are doing their job correctly ? Or is it recommended that a method be skeptical of everything and do a check something like:

 public void addEdge(T start, T dest, double length) {
            Map m = mGraph.get(start)
            if ( m ! = null)  ... ... 
        }

Once again, I am interested in knowing whats commonly done and whats ideally recommended.

JavaDeveloper
  • 5,320
  • 16
  • 79
  • 132
  • Is there any situation that mGraph.get may return null? If yes test for it or avoid to call it in that situation – ramaral Dec 31 '13 at 23:58
  • Your choice of a naming convention for the addEdge() method is odd... All you're doing there is getting an object from a graph. Why not name it getNode() or getEdge? – IgorGanapolsky Feb 24 '14 at 17:14

8 Answers8

3

Such debugging is part of the development process and should not be the issue at runtime.

Methods don't trust other methods. They all trust you. That is the process of developing. Fix all bugs. Then methods don't have to "trust". There should be no doubt.

So, write it as it should be. Do not make methods check wether other methods are working correctly. That should be tested by the developer when they wrote that function. If you suspect a method to be not doing what you want, debug it.

Martijn Courteaux
  • 67,591
  • 47
  • 198
  • 287
1

I don't know if I understood the question, but I think you could throw an Exception get method instead. See this example with ArrayList:

ArrayList<Integer> a = new ArrayList<Integer>();
a.get(1); # Index 1

this throws an exception:

Exception in thread "main" java.lang.IndexOutOfBoundsException: Index: 1, Size: 0
...

So in your case, you could throw an exception in your get() method:

public void T get(int i) {
     ...
    if (... == null)
        throw new Exception("No value found.");
}

and in the addEdge() method:

public void addEdge(T start, T dest, double length) {
    try {
        mGraph.get(start).put(dest, length);
    } catch(Exception exc) {
        System.out.println("Error: " + exc.getMessage());
    }
}
Christian Tapia
  • 33,620
  • 7
  • 56
  • 73
0

My 2 cents.

This is a loaded question imho. A rule of thumb I use to is see how this function will be called. If the caller is something I have control over then , its ok to assume that it will be called with the right parameters and with proper initialization.

On the other hand if its some client I don't control then it is a good idea to do thorough error checking.

smk
  • 5,340
  • 5
  • 27
  • 41
0

The addEdge is trusting more than the correction of the addNode method. It's also trusting that the addNode method has been invoked by other method. I'd recommend to include check if m is not null.

Andres
  • 10,561
  • 4
  • 45
  • 63
0

Typically this is bad practice. Since it is possible to call addEdge before addNode and have a NullPointerException (NPE) thrown, addEdge should check if the result is null and throw a more descriptive Exception. In my opinion, the only time it is acceptable not to check for nulls is when you expect the result to never be null, in which case, an NPE is plenty descriptive.

Windwaker
  • 111
  • 1
  • 9
0

That's where constructors come into play. If you have a default constructor (eg. with no parameters) that always creates a new Map, then you're sure that every instance of this class will always have an already instantiated Map.

Henrique
  • 4,921
  • 6
  • 36
  • 61
0

If it is in the same class it is fine to trust the method.

It is very common to do this. It is good practice to check null values in constructor's and method's arguments to make sure that nobody is passing null values into them (if it is not allowed). Then if you implement your methods in a way that they never set the "start" graph to null, don't check for nulls there.

It is also good practice to implement unit tests for your methods and make sure that they are correctly implemented, so you can trust them.

Viktor K.
  • 2,670
  • 2
  • 19
  • 30
0

I think your notion of "correctly" is a bit debatable. Your example seems to suggest that the returning of null is incorrect. A method could be quite correct in returning null and forcing you to account for that possibility somehow.

A well-written method will quite clearly define the domain of inputs and the range of outputs (a return type, exceptions under certain circumstances, null, a null object, etc.) possible for the method. There should also be a suite of tests that convey how the method is to be called and how all those possibilities can be accounted for.

In the real world, especially if you are on a project with developers from other organizations that may be less disciplined, you may not be so lucky. Programming defensively is fine, but I believe you as the client have to make sure you don't do anything that covers up an error in a method that violates its contract. Exceptions are a good thing in development; don't be afraid of letting them notify you when something went wrong somewhere. Then the developers can be made aware that their code needs to be strengthened to meet its contract.

Finally, since one of the tags on the question refers to Effective Java...on the subject of null in particular, you should test and document what you will do when a client calls your method with null--like throwing IllegalArgumentException. In your private methods, do an assert of your preconditions so you get AssertionErrors.

Vidya
  • 29,932
  • 7
  • 42
  • 70