0

I have a List of jobs that takes x number of steps (say 5). Each step must be successful to proceed, if any step fails, then a ticket must be raised and execution of current job must be forfeited and proceed with next job.

This is what i currently have (and it works like a charm).

for(Job aJob: jobs){
    // Step 1
    try{
        Obj objRet1 = method1();
        // use objRet1;
    }
    catch(Exception e){
        raiseTicket(errMsg1,e);
        break;
    }

    // Step 2
    try{
    Obj objRet2 =   method2();
    // use objRet2;
    }
    catch(Exception e){
        raiseTicket(errMsg2,e);
        break;
    }

    // Step 3
    try{
    Obj objRet3 = method3();
    // use objRet3;
    }
    catch(Exception e){
        raiseTicket(errMsg3,e);
        break;
    }

    ...

    // Step 5
}

This is not very elegant and easily readable, IMO. I would like to condense it to something like below.

for(Job aJob: jobs){
    step1();
    step2();
    ..
    step5();
}

step1(){
    try{
        ...
    }
    catch(Exception e){
        raiseTicket(errMsg1,e);
        break;
    }
}

step2(){
}
...

Could someone throw some light on how to improve this program? Please note, returning a value or storing it in method argument also may not work, since, what am trying to achieve here is to avoid the boiler plate code that is required to break execution and neatly package them in a readable method.

Cyriac George
  • 153
  • 2
  • 11

5 Answers5

1

First of all, break will not interrupt the current iteration but the whole loop. You need to use continue to achieve that. But since an exception that is thrown will skip over the remaining steps, in this case you don't need any extra statement.

Why not use something like this?

// create list with errorMessages in the following order: errMesFor1, errMesFor2,..., errMesFor5
List<String> messageList = new ArrayList<>();
messageList.add(errMesFor1);
messageList.add(errMesFor2);
messageList.add(errMesFor3);
messageList.add(errMesFor4);
messageList.add(errMesFor5);

for(Job aJob: jobs){
    // create list that holds successfully created objects 
    List<Obj> objList = new ArrayList<>();
    try {
            Obj objRet1 = method1();
            // use objRet1;
            list.add(objRet1);
            Obj objRet2 = method2();
            // use objRet2;
            list.add(objRet2);
            Obj objRet3 = method3();
            // use objRet3;
            list.add(objRet3);
            Obj objRet4 = method4();
            // use objRet4;
            list.add(objRet4);
            Obj objRet5 = method5();
            // use objRet5;
            list.add(objRet5);
        }
        catch(Exception e){
            // retrieve message for the first element that was not successfully added to the object list, hence the one that threw error
            raiseTicket(messageList.get(objList.size()),e);
        }

This way, you only have to write the try-catch block once.

It is also nice to organize the messages in a list (or you could even write a custom wrapper class over a list).

The only extra thing that is required is the object list so that you can easily find the object that threw exception.

aUserHimself
  • 1,589
  • 2
  • 17
  • 26
1

What you could do is throw a checked exception in each of the step if an error occurs, and then catch the checked exception in the for loop. Something like this:

class StepException extends Exception {...}

for (Job aJob: jobs) {
  try {
     step1();
     step2();
     ...
     step5();
  } catch (StepException se) {
     // do something, e.g. print error to console
     // the for loop will keep going when this occurs
  }
}

step1() throws StepException {
   try {
      ...
   } catch (Exception e) {
      raiseTicket(...);
      throw new StepException(e);
   }
}

// similar to step2(), ..., step5()
Dat Nguyen
  • 1,626
  • 22
  • 25
1

One possible solution could be:

for (Job job : jobs) {
    AtomicInteger step = new AtomicInteger();
    try {
        Obj result = executeJob(step, () -> method1());
        // do something with result
        result = executeJob(step, () -> method2());
        // do something with result
        result = executeJob(step, () -> method3());
        // do something with result
    } catch (Exception e) {
        raiseTicket(errorMessages.get(step.get()), e);
    }
}

private Obj executeJob(AtomicInteger step, Supplier<Obj> supplier) {
    step.incrementAndGet();
    return supplier.get();
}

while errorMessages is a Map<Integer, String>.

Harmlezz
  • 7,972
  • 27
  • 35
0

As per JAVA basics No One Can Use break outside loop or switch

As Methods are represent stack memory entries you can't break loop from stack memory in case where loop is in another stack entry and method is in other stack entry.

Abhishek
  • 3,348
  • 3
  • 15
  • 34
0

Let the steps throw a specific exception and let the loop code handle it!

With the following exception class

class StepExecutionException extends RuntimeException {
    StepExecutionException(String message, Throwable cause) {
        super(message, cause);
    }

    void raiseTicket() {
        // Code that raises the ticket ...
        // The message can be retrieved with getMessage().
        // The cause can be retrieved with getCause().
    }
}

you can easily implement the steps with that code:

void step1() {
    try {
        ...
    } catch(Exception e) {
        throw new StepExecutionException("Step 1", e);
    }
}

And now your loop code looks like:

for (Job aJob : jobs) {
    try {
        step1();
        step2();
        ...
    } catch (StepExecutionException see) {
        see.raiseTicket();
    }
}
Seelenvirtuose
  • 20,273
  • 6
  • 37
  • 66
  • i like this solution. i see that Dat Nguyen also suggested same answer. so looks like its a common problem and there is a design pattern to solve it. – Cyriac George Apr 12 '17 at 06:33
  • may i ask, why RuntimeException is used and not a checked exception? – Cyriac George Apr 12 '17 at 06:34
  • `StepExecutionException` is a checked exception :) See this SO thread for some more info: http://stackoverflow.com/questions/6115896/java-checked-vs-unchecked-exception-explanation – Dat Nguyen Apr 12 '17 at 07:38
  • You can make it checked or unchecked - whatever you prefer. Joshua Bloch's book "Effective Java" is now somewhat aged. And there is a long-going discussion whether checked exceptions should be used at all. I personally prefer to only use unchecked exceptions. The main reason is to not clutter method signatures. – Seelenvirtuose Apr 12 '17 at 09:13