Why cant I return the result of ternary operator?

Question

Consider this code:

private static Context myContext;
public static Context getInstance() {
    myContext = myContext == null ? new Context() : myContext;
    return myContext;
}

After refactoring like this, my app started to throw NullPointers:

private static Context myContext;
public static Context getInstance() {
    return myContext == null ? new Context() : myContext;
}

Is it returning myContext and ignoring all after '=='? Can somebody explain?

EDIT: Did a bit more research on this, turns out that it was my mistake: seems that refactored code failed to assign the value to a Context class field.

Sorry for inconvenience.

If someone is still interested, here is the working code snippet:

package pckg;
import org.junit.Test;

public class TernaryTest {
    @Test
    public void testOK(){
        Context.getInstance().initialize();
        Context.getInstance().setReport(new Report());
        Context.getInstance().getReport();
    }
    @Test
    public void testFail(){
        Context.getInstanceRefactored().initialize();
        Context.getInstanceRefactored().setReport(new Report());
        Context.getInstanceRefactored().getReport();
    }
}

class Context{
    private static Context myContext;
    private boolean isInitialized;
    private Report report;
    public static Context getInstance() {
        myContext = myContext == null ? new Context() : myContext;
        return myContext;
    }
    public static Context getInstanceRefactored() {
        return myContext == null ? new Context() : myContext;
    }
    private void checkInitialized() {
        if (!isInitialized) {
            throw new IllegalStateException("Not initialized.");
        }
    }
    public void initialize() {
        if (isInitialized) {
            throw new IllegalStateException("Not initialized.");
        }
        isInitialized = true;
    }
    public void setReport(Report report) {
        this.report = report;
    }
    public Report getReport() {
        checkInitialized();
        return report;
    }
    }
    class Report{}

Your second version never initialize `myContext`. If you don't have other ways to initialize it than calling `getInstance()` and you are using a method inside your class which uses `myContext` or a method that return the `myContext` instance on which you are performing some operations, then yes, it will throw a NPE somewhere. — Alexis C., Dec 09 '14 at 17:02
Please show a short but *complete* program demonstrating the problem. — Jon Skeet, Dec 09 '14 at 17:03
Try adding brackets containing everything after return and before semicolon. — EDToaster, Dec 09 '14 at 17:04
Try with the brackets as things within brackets first get run after that you can return value of myContext — foxt7ot, Dec 09 '14 at 17:07
@YaseenKhan: No, brackets don't affect execution order - they affect grouping. The problem here isn't precedence - it's something the OP hasn't shown us, such as a use of `myContext` directly (which will be null, as per ZouZou's comment). — Jon Skeet, Dec 09 '14 at 17:08
Or in other words, your second version will always return a new instance (which is *never* `null`). And it seems that your code can’t handle that, most probably because one piece of code assumes that another has initialized a certain property of `Context`. For further diagnosis you have to look at the stack trace of your `NullPointerException` and the code indicated by the stack trace. — Holger, Dec 09 '14 at 17:15

Drew Kennedy · Accepted Answer · 2014-12-09T17:34:04.577

1

The problem is you have created an instance of myContext, but in the second version you never assign it a value, but instead create an anonymous instance, which is not bound to your higher-scoped variable myContext.

private static Context myContext;
public static Context getInstance() {
    return myContext == null 
       ? myContext = new Context() //it is now assigned
       : myContext;
}

edited Dec 09 '14 at 17:34

answered Dec 09 '14 at 17:20

Drew Kennedy

4,118
4
24
34

1

It doesn’t “throw away” the new instance, it returns it. I’d assume that using the instance that this method returns is even the primary intention of this method. That the code breaks once `getInstance()` returns a new instance all the time is an indicator for serious problems outside the scope of the question; so your answer tells the OP how to make the `NullPointerException` disappear but won’t solve the bigger problem. – Holger Dec 09 '14 at 17:26
Consider the first method that was used by the OP. He was assigning a new instance to his higher-scoped variable. In the second case (ternary), he was creating a new instance every time. I think it's safe to assume this was not the intention when looking at how he was `lazy-loading` before refactoring his code. You are right about it not being "thrown away". It was the intent of saying that it was not being bound to his instance variable. I'll re-word that. – Drew Kennedy Dec 09 '14 at 17:32
Right, it’s surely not intentional that a new instance is created all the time but it seems to be good enough to spot the problem that using lazy creation and relying on one piece of code having already performed a certain action on that lazily created instance at some point of time is rather contradictory. It’s very likely that this inconsistency will cause problems in the future even when the bug of creating a new instance all the time has been solved. After all the application has some decoupled pieces of code but still relies on a particular order of execution. – Holger Dec 09 '14 at 17:47
I failed to assign the value to a field. So that is the answer. – MrAs Dec 11 '14 at 10:49

Why cant I return the result of ternary operator?

1 Answers1