Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have this problem called out by PMD (static code analyzer) more often that I would like, and would like to know how someone with more experience than me would write it. My code is functional but I find it inelegant, so I want to know how other programmers would write this piece.

The case is, in a network/IO petition I may or may not get a result from, but my parent method is not null-proof so I always have to return something. I also don't like several returns on a method.

public String getBingLocation(Coordinate... data)
{
    String response = "Not Retrieved";
    final Coordinate location = data[0];
    JSONObject locationData;
    try {
        locationData = NetworkManager.getJSONResult(ApiFormatter
                .generateBingMapsReverseGeocodingURL(location.Latitude, location.Longitude));
        if (null != locationData) {
            final String address = this.getAddressFromJSONObject(locationData);
            response = address;
        }
    } catch (final ClientProtocolException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        return response;
    } catch (final JSONException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        return response;
    } catch (final IOException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        return response;
    } finally {
        location.Street = response;
    }
    return response;
}

Other example:

public static Object loadObject(final String fileName, final Context context) {
    Object object = null;
    try {
        ObjectInputStream objectInputStream = null;
        try {
            final FileInputStream fileStream = context.openFileInput(fileName);
            objectInputStream = new ObjectInputStream(fileStream);
            object = objectInputStream.readObject();
        } catch (final ClassNotFoundException catchException) {
            LoggerFactory.consoleLogger().printStackTrace(catchException);
        } catch (final ClassCastException catchException) {
            LoggerFactory.consoleLogger().printStackTrace(catchException);
        } catch (final Exception catchException) {
            LoggerFactory.consoleLogger().printStackTrace(catchException);
        } finally {
            if (objectInputStream != null) {
                objectInputStream.close();
            }
        }
    } catch (final IOException catchException) {
        LoggerFactory.consoleLogger().printStackTrace(catchException);
    }
    return object;
}
share|improve this question
5  
What is the question? What do you mean by "DD anomalies"? –  Simon Feb 1 '13 at 10:18
    
stackoverflow.com/a/1615477/1266326 <- How to avoid those. My code is functional but I find it inelegant, so I want to know how other programmers would write this piece. –  Ef Es Feb 1 '13 at 10:23
    
pmd.sourceforge.net/pmd-4.2.6/rules/controversial.html <- search for DataflowAnomalyAnalysis –  Ef Es Feb 1 '13 at 10:24
    
Thanks for the links –  Simon Feb 1 '13 at 10:27

2 Answers 2

Part of the problem is your functions have more than one responsibility. They are both getting a result and handling errors. You're also getting unnecessarily hung up trying to avoid multiple return points, which is a vestigial practice from C programming where it can cause memory leaks. You should use multiple returns if it clarifies your code, especially in short functions. I'm also guessing that your logging code is getting repeated all over the place. If you separate those concerns and try to avoid repeating yourself, you get something like this:

public JSONObject getLoggedJSONResult(String api)
{
    try {
        return NetworkManager.getJSONResult(api);
    } catch (final ClientProtocolException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
    } catch (final JSONException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
    } catch (final IOException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
    }

    return null;
}

public String getBingLocation(Coordinate... data)
{
    final Coordinate location = data[0];

    JSONObject locationData = getLoggedJSONResult(
       ApiFormatter.generateBingMapsReverseGeocodingURL(
          location.Latitude, location.Longitude));

    if (null == locationData) {
        return "Not Retrieved";
    } else {
        return this.getAddressFromJSONObject(locationData);
    }
}

As a general principle, most DD anomalies can be fixed by splitting the function. It's mostly a matter of figuring out where to split it.

Your second example is trickier. That sort of situation is why try-with-resources statements were invented. You can use an early return to eliminate the DD anomaly on object, but you're not going to be able to do much about objectInputStream without a try-with-resources.

share|improve this answer
    
I really like your answer, will probably give it as correct in a couple of days. My experience is short and most of my code does too much, as you say, and is far from generic or reusable. Every time I read code from someone smarter than me it looks cleaner, more simple and complete, yet has a lot of subclassing and methods in every file. I want to know if this solutions (mixed with others) won't make the classes grow too much in size over time, and they're so atomic they make future maintenance a quest to find a method in the file or where it's called from. I know IDE already do that but still –  Ef Es Feb 5 '13 at 20:50
2  
Let me put it this way. Your solution and mine have approximately the same lines of code. Is it easier to go back later and find a certain spot in the middle of a function, or to find an entire well-named function? Also, functions like getLoggedJSONResult can likely be reused in other places, making the code even shorter. Yes, you sometimes have to trace through more functions, but if you name them well you usually don't. For example, I understood your code just fine without needing to know the details of generateBingMapsReverseGeocodingURL. –  Karl Bielefeldt Feb 5 '13 at 21:34
1  
Further to @KarlBielefeldt's comment, keep an eye on whether the methods in your classes all belong together. You should find more opportunities to split classes, which should help keep each class manageable and coherent. –  Baqueta Feb 7 '13 at 11:25

As for your first example, I'd write it like this :

public String getBingLocation(Coordinate... data)
{
    String response;
    final Coordinate location = data[0];
    JSONObject locationData;
    try {
        locationData = NetworkManager.getJSONResult(ApiFormatter
                .generateBingMapsReverseGeocodingURL(location.Latitude, location.Longitude));
        if (null != locationData) {
            final String address = this.getAddressFromJSONObject(locationData);
            response = address;
        } else {
            response = "Not Retrieved";
        }
    } catch (final ClientProtocolException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        response = "Not Retrieved";
    } catch (final JSONException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        response = "Not Retrieved";
    } catch (final IOException e) {
        LoggerFactory.consoleLogger().printStackTrace(e);
        response = "Not Retrieved";
    } finally {
        location.Street = response;
    }
    return response;
}

The main idea is to avoid overriding a value without reading it (this means one assignement is useless because never read).

A solution is to have a single return statement, and return a non initialized variable. Then go through all possible path and check that a value is always assigned, and no more than once.

share|improve this answer
    
With this implementation if the method throws an exception the return statement is never reached, so the caller receives a null. The solution would be a return in the finally block, but that practice is heavily not recommended. –  Ef Es Feb 5 '13 at 14:43
    
if an exception in one of the catch block is thrown, then Yes the return statement will be reached. Just try it out in debug mode with a break point. –  XGouchet Feb 5 '13 at 15:21
    
You are right, but I got a new error: "The local variable response may not have been initialized" –  Ef Es Feb 5 '13 at 15:32
    
you just need to check that all possible paths lead to a response value... and my bad, you have to add an else statement in your if (null != locationData) to set a response –  XGouchet Feb 5 '13 at 15:36
    
It still throws the same error. The problem is not the if clause but the try. The object has to be initialized before because it assumes the code may fail anytime, even if the assignation is the first statement. –  Ef Es Feb 5 '13 at 15:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.