NullPointerException - Trying to access object before it's properly instantiated?

Question

Currently having a problem with an android app where I think the thread is trying to use an object before it's properly instantiated.

Here's the constructor for my class:

String name;
String price;
String percentChange;
String peRatio;

private String ticker;
private JSONObject stockQuote;
public Stock(String ticker) {
    this.ticker = ticker;

    // Build query and URL.
    String url = "http://query.yahooapis.com/v1/public/yql?q=";
    String query = "select%20*%20from%20yahoo.finance.quotes%20where%20symbol%20in%20(\"" +
            ticker +
            "\")&env=store://datatables.org/alltableswithkeys&format=json";

    url += query;

    stockQuote = fetch(url);
    name = get("symbol");
    price = get("Ask");
}

Here's the fetch() function being used to retrieve a JSONObject from a HTTP response:

private JSONObject fetch(String myurl) throws IOException {
    InputStream is = null;
    String contentAsString = "";

    try {
        URL url = new URL(myurl);
        HttpURLConnection conn = (HttpURLConnection) url.openConnection();
        conn.setReadTimeout(10000 /* milliseconds */);
        conn.setConnectTimeout(15000 /* milliseconds */);
        conn.setRequestMethod("GET");
        conn.setDoInput(true);
        // Starts the query
        conn.connect();
        int response = conn.getResponseCode();
        is = conn.getInputStream();

        // Convert the InputStream into a string
        contentAsString = readIt(is);

        // Makes sure that the InputStream is closed after the app is
        // finished using it.
    } finally {
        if (is != null) {
            is.close();
        }
    }

    JSONObject queryResult = null;

    try {
        JSONObject json = new JSONObject(contentAsString);
        queryResult = json.getJSONObject("query").getJSONObject("results").getJSONObject("quote");
    } catch (JSONException e) {
        System.err.println("Failed to decode JSON");
    }

    return queryResult;
}

and finally here's the get() method, used to retrieve a string from the JSONObject:

public String get(String key) {

    String value = null;

    try {
        value = stockQuote.getString(key);
    } catch (JSONException e) {
        System.err.println("Couldn't find " + key);
    }

    return value;
}

A NullPointerException gets thrown by "value = stockQuote.getString(key);" - sometimes, but not everytime. I assume this is because it's trying to access the stockQuote before the HTML response has been parsed?

I'm guessing there's an obvious solution to this, but I haven't encountered this problem before. Any ideas?

do you ever catch the exception from the first `try`-block? Seems to me the HTTP-connection can fail silently — simon, Dec 09 '15 at 21:35
try changing `System.err.println("Failed to decode JSON");` to `Log.e("Failed to decode JSON")` and add a similar log statement at the start of the "try" block `Log.e("attempting to parse JSON")`. I would almost bet money that in those times where you get the null pointer, your parsing is failing and thereby you will see the failure message in the console at those times. — gMale, Dec 09 '15 at 21:37
Look up "unsafe publication". You're passing out references to the object before the constructor is finished. — chrylis -cautiouslyoptimistic-, Dec 09 '15 at 21:42
@07acrow, is the exception caused by the `get()` called within the constructor? If so, @gmale is probably right. Otherwise, you should try making `stockQuote` `final` so it's visible to other threads. — shmosel, Dec 09 '15 at 21:46
BTW, could `get(String)` be more easily implemented as `return stockQuote.optString(key, null);`? — Andy Turner, Dec 09 '15 at 22:13

Andy Turner · Answer 1 · 2015-12-09T22:18:24.630

I suspect that the main issue here is one of visibility, as @shmosel said in the comments above: because stockQuote is non-final, the assigned value is not guaranteed to be visible to all threads after once the constructor completes. Making stockQuote final should fix this.

Can I suggest that doing work in the constructor like this is a bad idea: it makes it difficult to test (Miško Hevery's article on this is a good read), and you are calling alien methods in the constructor, which is discouraged (e.g. by Java Concurrency In Practice).

Instead, you can move the work (downloading the ticker) into a static factory method, which has the added benefit of fixing your race condition:

public static Stock create(String ticker) {
  // Build query and URL.
  String url = "http://query.yahooapis.com/v1/public/yql?q=";
  String query = "select%20*%20from%20yahoo.finance.quotes%20where%20symbol%20in%20(\"" +
        ticker +
        "\")&env=store://datatables.org/alltableswithkeys&format=json";

  url += query;

  JSONObject stockQuote = fetch(url);

  return new Stock(ticker, stockQuote);
}

private Stock(String ticker, JSONObject stockQuote) {
  this.ticker = ticker;
  this.stockQuote = stockQuote;

  if (stockQuote == null) throw new NullPointerException();

  name = get("symbol");
  price = get("Ask");
}

In this way, you can't have an instance of Stock before stockQuote is available, so you cannot get a NullPointerException when calling get. (get should also be made final, so that it is not an alien method either).

(Of course, this doesn't work so easily if you need to subclass Stock.)

You can also test the Stock class more easily, since you can directly inject a JSONObject, rather than having to download a "real" one (if you make the constructor non-private).

+1 for the static factory method. But it doesn't really affect the visibility issue. Either approach would work correctly with (and only with) a `final` field. — shmosel, Dec 09 '15 at 22:14
@shmosel I suspected that might be the case. I will edit to give that more prominence (and props to you). — Andy Turner, Dec 09 '15 at 22:16
@07acrow: If you take this route, I'd suggest an `IllegalArgumentException` (plus an explanatory message, of course) over a NPE. I think the constructor changes here are correct but I would advise against the static create(String) method. Not only is it non-descriptive, it also seems to want to live inside another object. I agree with the gist of what's being said here. You have 2 collaborators: a stock and its service. Separating them would seem to make sense and help fix the deeper issues that are causing your problem. — gMale, Dec 11 '15 at 16:38
@gmale sure, you can. I am used to using Guava's `Preconditions.checkNotNull` for the convenience of using the method return to assign the field; I just didn't want to introduce an unnecessary library :) — Andy Turner, Dec 11 '15 at 16:43

NullPointerException - Trying to access object before it's properly instantiated?

1 Answers1