How to clean a refactor Java for-if-try-catch-else kind of messy code

Question

Often I came across situations like this, how to write this code in a neat and clean way. One more issue I find here is performance as I am iteration a list and then it's properties.

Edit : - while the suggested How to tackle a 'branched' arrowhead anti-pattern? post address the nested if-elseif-if concern to a great extends. Still, I am looking for an answer for how to handle multiple try catch in (if any) in the code, and how to improve performance.

for (String item : someList) {
    IResultOne resultOne = doSomething(item);

    if (resultOne != null) {
        for (SomeObject obj :resultOne.getSomeOtherList) {
            IResultTwo resultTwo = doMegaProcessing(obj);

            if(resultTwo != null) {
                try {
                    doSomeMoreProcessing(resultTwo);
                } 
                catch (CanNotProcessingException ex) {
                    errorObj.addError("can't process reason");
                }
            } 
            else {
                errorObj.addError("item does not have {} error ");
            }
        }
    } 
    else {
        errorObj.addError("item is invalid "+item)
    }
}

For what it's worth, cleaning up your code formatting will help tremendously. See my edit to your code. — Robert Harvey, Jun 27 '18 at 18:48
Removing the obvious syntax errors also helps (did so). That could make your question eligible for codereview.stackexchange (AFAIK they don't accept code which does not compile). — Doc Brown, Jun 27 '18 at 18:52
Don't know how tractable this is in Java, but promises and continuations would make this a lot cleaner. — Robert Harvey, Jun 27 '18 at 18:58
Is there a reason for the javascript tag? I also think this would be better for codereview.stackexchange — Hangman4358, Jun 27 '18 at 20:00

Doc Brown · Answer 1 · 2018-06-28T05:25:47.230

1

First, I would use sentinel conditions instead of nested those if/else blocks. That reduces the indentation, and will already make the code block more readable. Second, the try/catch block is probably better refactored to a function which directly returns just the error string instead of throwing an exception.

In total, this leads to something like this:

for (String item : someList) {
    IResultOne resultOne = doSomething(item);
    if (resultOne == null) {
        errorObj.addError("item is invalid "+item)
        continue;
    }
    // ...
    for (SomeObject obj :resultOne.getSomeOtherList) {
        IResultTwo resultTwo = doMegaProcessing(obj);
        if(resultTwo == null) {
            errorObj.addError("item does not have {} error ");
            continue;
        }

        String errorText = trySomeMoreProcessing(resultTwo);
        if(errorText!=null)
            errorObj.addError(errorText);
    }
}

 String trySomeMoreProcessing(IResultTwo resultTwo) {
     try {
        doSomeMoreProcessing(resultTwo);
     } 
     catch (CanNotProcessingException ex) {
          return "can't process reason";
     }
     return null;
 }

Furthermore, I would consider to refactor parts like the inner loop to a function on its own, and give that function a sensible name.

edited Jun 28 '18 at 05:25

answered Jun 27 '18 at 19:09

Doc Brown

199,015
33
367
565

Thanks.. This looks cleaner to read and is helpful indeed – ifelse.codes Jun 27 '18 at 19:16
1

Btw I read the question, noticed gnat's duplicate suggestion, remembered the typical refactoring pattern for that problem, and was then surprised to read exactly the code I was thinking about in your answer. I take this as a strong indication that gnat's dupe was very much not “nonsense” (at least in this case, I understand your general frustration). – amon Jun 27 '18 at 20:28
@amon: ok, I admit, after reading the answer again, I can see some similarities (but probably far too few to be really useful for the OP of this question). – Doc Brown Jun 27 '18 at 20:50
I agree with the first part, but I think the `try`-`catch` is fine. (Exceptions exist for a reason!) – Solomon Ucko Jun 27 '18 at 21:18
@SolomonUcko: you missed my point, I did not suggest to remove the try-catch, I suggested to refactor the mapping from try-catch to a string into a separate function (see my edit). There is no change in semantics to the original code. – Doc Brown Jun 28 '18 at 05:26
I see. What's wrong with the explicit `try`-`catch`, though? – Solomon Ucko Jun 28 '18 at 12:55
@SolomonUcko: the "try-catch" produces some "noise" for the reader which can be "abstracted-away" to gain a little bit more readability, that's all. – Doc Brown Jun 28 '18 at 12:59
IMHO, it's less readable this way. It obscures the intentions of the code. – Solomon Ucko Jun 28 '18 at 13:58
@SolomonUcko: that is a question of naming. Names like `doSomeMoreProcessing` are not really useful for discussing about the intentions of code. – Doc Brown Jun 28 '18 at 14:26
What would be a better example? Also, should this be moved to chat? – Solomon Ucko Jun 28 '18 at 23:40
Let us [continue this discussion in chat](https://chat.stackexchange.com/rooms/79546/discussion-between-doc-brown-and-solomon-ucko). – Doc Brown Jun 29 '18 at 05:25

How to clean a refactor Java for-if-try-catch-else kind of messy code

1 Answers1