If else conditionals in getters

[Geane Norm]

Hi,
I have a getter method that looks like this:

 public String getIp() {     
            return ipAddress;
    }

Inside the get method I would like to check whether the ipAddress is null and in case assign it to an Empty string, otherwise return the ipAddress. My main question is: "Is a good practice to add if else statements in a getter?" If so, would this be the correct way to go about?

public String getIp() {
        if (ipAddress == null) {
            ipAddress = new String("");
        } else {
            return ipAddress;
        }
        return ipAddress;
    }

Thank you

[Junilu Lacar]

You can ask yourself a different question: Does it make sense? Would someone using the object to which such a getter belongs be surprised somehow by that behavior? For example, is the following something you would reasonably expect?

Device myConnectedDevice = new Device();
myConnectedDevice.setIp(null);
...
if (myConnectedDevice.getIp() == null) {
   System.out.println("This will NOT print");
} else {
   System.out.println("This WILL print");
}

[Junilu Lacar]

Geane Norm wrote:My main question is: "Is a good practice to add if else statements in a getter?" If so, would this be the correct way to go about?

These questions are a good sign: They indicate that you're thinking carefully about your program's design.

I've already given a couple of my rules of thumb when designing an API: "Does it make sense?" and "Will that behavior surprise anyone?" These two questions often lead me to "Does the resulting code read well?" and/or "Does the resulting code tell a good story?" In turn, those often lead me to trying to answer the question, "What would ideal code look like then?"

My guess is that you already have a sense that the if-else in a getter is not such a good idea; you just don't know what other alternative design choices you have. Here are some:

1. Make null an illegal value: Make the setIp() reject a null value with an IllegalArgumentException

2. Use a reasonable default IP value, like maybe "localhost" or "127.0.0.1" or for IPV6, "::1" .  You will want to at least document this in your API.  This means, you'll have this kind of code:

public class MyDevice ... {

    private static final String LOOPBACK = "127.0.0.1";

    // fields
    private String ipAddress = LOOPBACK; // default
    private ... 
    ...

    /**
     * Returns this device's IP address. 
     * If the IP has not been explicitly set, will default to "127.0.0.1"
     */
    public String getIp() { ... }
}

I'm sure there are other alternatives you can think of besides these but these are good enough to get you thinking about what other options you can explore. Use the Principle of Least Astonishment (POLA) to guide your choice.

[Paweł Baczyński]

Also, there is no sense in using the String constructor. If you want to assign a String to a variable just write:

ipAddress = "";

[Ben Ernstberger]

By the way: the else block is unnecessary.

[Tim Holloway]

Paweł Baczyński wrote:Also, there is no sense in using the String constructor. If you want to assign a String to a variable just write:

ipAddress = "";

Or better yet, simply initialize the object  propertyto a default address of "" and forget about conditional logic in the getter entirely.

A getter should be idempotent. That is, repeated calls to the getter should return the same value (assuming that the property being retrieved hasn't been set by an external function). It should not have any "side effects" (changing its own or other property values) or invoke external functions.

A getter should be low-overhead. In JavaServer Faces, for example, a property might be retrieved up to 6 times (or more) for a single web page request.

A getter, in short, should avoid complicated logic, and especially branching logic, since among other things, that means more complicated testing is required to validate the property in question when doing unit testing.

This doesn't mean that a getter can only be straight-line value fetch-and-return. In JSF, I often have getters tied to items pulled from a database. In that case, the first call sees no data, pulls data from the database and then caches it so that subsequent calls don't have to fetch the data again (remember what I said about JSF's multiple invocations of property methods!). Another common case I have is cascaded selection lists - for example, country, state, city, where changing the state has to change the list of available cities. In that case, my state property-change listener invalidates the cache of cities so that the next city property-fetch will go back the the backing store for a new list of cities (which it will then cache).

That may sound like yes, I'm promoting logic in my "get" methods, but the logical effect is the same as a straight fetch, which is what's really important.

[Geane Norm]

Hi,
thank you for all replies, much appreciated. I have ended up with something along these lines:

public static final String DEFAULT_VALUE = "";
private String ipAddress;

public String getIp() {
        if (ipAddress == null) {
            ipAddress = DEFAULT_VALUE;
        } else {
            return ipAddress;
        }
        return ipAddress;
    }

In turn my setIp will look like this:

public void setIp(String ipAddress) {
        if (ipAddress == null) {
            throw new NullPointerException("IpAddresses cannot be null");
        } else {
            this.ipAddress = ipAddress;
        }

    }

Cheers,

[Liutauras Vilda]

So, you haven't solved anything yet.

1. Method name getIp() isn't clear enough. getIPAddress() would be better, or even getIPv4Address(), because IP is Internet Protocol, and not the address itself.
2. Your setIp() lets you set address which reads as "evenDodgyIPAddressIsAllowed". You may want to validate it (to some extent at least).
3. Your current getters implementation doesn't need else clause along with its block. Think about it, if ipAddress is null, then it would assign empty string and would return it (line 7). If it wouldn't be null, it would return yet again from line 7 (if you were not have "else" block). Why do you need "else" block to have it as a different escape door, which go to the same room?

Question: how empty IP address is better than null? Both are invalid addresses. How empty string helps you deal better with a non existing IP address?

[Campbell Ritchie]

To add to Liutauras' criticisms:

1: Your second version of the method breaches one of the principles Junilu gave you: that method has a side‑effect.

2: I prefer the version with the exception if the IP is null, but again Junilu is right: throw the exception before the null gets into your object, otherwise your object is already in an incosistent state and doesn't maintain its invariants.

The old Sun style guide suggests not to use if‑else for returning values; that would preclude exceptions and side‑effects. Another thing about returned values: if you are returning a mutable reference type, take a defensive copy first.
Why are you using a String which is probably an inappropriate return type? Is there no ready‑made class for IPv4 addresses, or could you create one? You should probably use that instead.

[Dave Tolls]

Since the setIp method implies that a null ipAddress is invalid then surely it would be preferable to write the class such that it can never have a null ipAddress.

At the moment, with the getIp method, you are returning what would be an invalid ipAddress (an empty string).  That is almost certain to break something, and not in an obvious way like with a NullPointerException.

[Campbell Ritchie]

Last night, I wrote:. . . Is there no ready‑made class for IPv4 addresses . . .

Of course there is, but I couldn't think of its name at the time.