Check address from Melissa server

How can I improve this code?

```
public String checkAddressFromMelissaServer(boolean isEditMode, boolean addressChecked, Address address) {
boolean isAddress = false;
boolean isError = true;
String SystemResult = "";
boolean systemErrorFlag = false;
StringBuffer requestAddress = new StringBuffer();
Address returnAddress = null;
StringBuffer addressNew = new StringBuffer();
StringBuffer addressReturn = new StringBuffer();

try {
if (address != null && address.getHouse() != null) {
requestAddress.append(address.getHouse());
}
if (address != null && address.getStreetPrefix() != null) {
requestAddress.append(" ").append(address.getStreetPrefix());
}
if (address != null && address.getStreet() != null) {
requestAddress.append(" ").append(address.getStreet());
}
if (address != null && address.getStreetType() != null) {
requestAddress.append(" ").append(address.getStreetType());
}
if (address != null && address.getStreetDirection() != null) {
requestAddress.append(" ").append(address.getStreetDirection());
}
if (address != null && address.getCity() != null) {
requestAddress.append(" ").append(address.getCity());
}
if (address != null && address.getCity() != null) {
requestAddress.append(" ").append(address.getCity());
}
if (address != null && address.getProvince() != null) {
requestAddress.append(" ").append(address.getProvince());
}
if (address != null && address.getPostal() != null) {
requestAddress.append(" ").append(address.getPostal());
}

AddressVerificationAdaptor addressVerificationAdaptor = getAddressAdaptor();
if (a

This code could be more efficient, in that it could be written more compactly. However, I don't see anything that would cause a noticeable performance impact. If you're connecting to a remote server to verify an address, I would expect that the time to send, process, and receive that request would dwarf whatever time it takes to concatenate a few strings.

I have much deeper concerns about this code than performance.

Error handling

This line, in particular, has me very worried:

} else {
    addressNew = new StringBuffer(returnAddress.getErrorString());                                                      
}

Always distinguish error messages from legitimate data. Do not output them through the same channel. Here's a real-life example of what happens when error messages are accidentally treated as data:

That's Welsh for "I am not in the office at the moment. Send any work to be translated."

Don't let that happen to users of this function. In Java, the right thing to do when you encounter an error is to throw an exception. That forces a caller somewhere in the call stack to deal with the error as an error, and helps prevent mix-ups like this.

Return values

It's not clear what this function is supposed to return. Here's the return for the "desired" code path:

String returnValue = "Success" + ";" + isAddress + ";" + addressNew + ";" + requestAddress + ";" + SystemResult + ";" + systemErrorFlag
        + ";" + toshowErrorCode + ";" + additionalErrorCodes + ";" + isEditMode + ";" + isError + ";" + addressReturn;
return returnValue;

However, there is one possible failure scenario:

String returnValue = "Error";
return returnValue;

… and a third possible outcome:

} catch (AddressVerificationException addressException) {
    LOGGER.error("", addressException);
}
return null;

The existence of these three possibilities makes your function difficult to use correctly. The caller will need to check whether the null, and if it's not null, whether the text starts with "Success;", and if so, interpret the result by splitting the text at semicolons. There's a good chance that the caller will be sloppily optimistic and neglect to handle the two error conditions.

Never return null to indicate failure. It may be OK to use null to signify "no such value exists" — for example, that there is no street prefix. Failures need to be conveyed through separate channels, as discussed above. For that matter, why should getAddressAdaptor() ever return null? By returning null instead of throwing an exception, getAddressAdaptor() is foisting on this function the same burden that this function is foisting on its caller.

A semicolon-delimited result is also unwieldy. It would be better to return an AddressVerificationResult object. If the result really does need to be in string form because it will need to be transmitted over a stream, then use a standard serialization format like json, xml, or yaml.

Offload the string concatenation code

All of that code to concatenate the various Address fields into a string should be moved into a toString() method of the Address class.

In the part of the code where you do this…

if (returnAddress.getHouse() != null && returnAddress.getHouse().length() > 0)
    addressNew.append((returnAddress.getHouse() == null) ? "" : returnAddress.getHouse());

… the ternary expressions are pointless, because you've already ascertained that getHouse() is non-null. Also, be sure to use braces consistently for all if-statements.

Keep it simple

What are the parameters isEditMode and addressChecked for? What influence could they possibly have over whether a given address is valid or not? It seems that they just end up being plopped into fields within the semicolon-delimited result string.

Simplify this function by having it do one thing only — and get rid of irrelevant complications.

StackExchange Code Review Q#92720, answer score: 6