1
\$\begingroup\$

I have a situation in a class tries to create a group of same type of components and if some component fails then it needs to decide whether it should continue or throw the exception. Now the behavior whether to continue or re-throw depends in the context in which that class is used.

So a obvious solution was to pass the behavior from outside in a constructor, which is effectively a strategy pattern. For now I had two implementations:

  • ContinuePolicy
  • AbortPolicy

These both policies were stateless so I created a single instance for them. Now I needed a strategy called SingleSuccessPolicy ( which ignores exception if atleast one of the components is successfully created). Now the problem is that to know whether all are failing or there is at least one success, the strategy needs to store some state (possibly count of components to be created). The side effect of it is that it makes creating a single instance infeasible.

Java Code:

class GroupComponent {
   private final ComponentExceptionPolicy exceptionPolicy;
   public GroupComponent(ComponentExceptionPolicy policy, GroupData data) {
      this.exceptionPolicy = policy;
   }

   public void createComponents() throws Exception {
      for(ComponentData compData : data.getComponentsData()) {
         try { 
           createComponent(compData)
         } catch (Exception e) {
           policy.apply(compData, e);
         }
      }
   }
}

Code for Strategies:

public interface ComponentExceptionPolicy {
    public void apply(ComponentData data, Exception e);
}

class ContinuePolicy extends ComponentExceptionPolicy {
    @Override
    public void apply(ComponentData data, Exception e) throws Exception {
      //just log it and continue
    }
}

class AbortPolicy extends ComponentExceptionPolicy {
    @Override
    public void apply(ComponentData data, Exception e) throws Exception {
       throw e;
    }
}

class MaximumFailuresPolicy extends ComponentExceptionPolicy {
   private int n; //the expected size of group component

   MaximumFailuresPolicy(int n) {
       this.n = n;
   }

   @Override
   public void apply(ComponentData data, Exception e) {
      //on each exception I decrement count
      if(--n == 0) { //check if exception threshold is reached
         //as soon as found that all have failed then exception is thrown,
         //if any of component succeeds, then n will not reach to 0 and it
         //will just continue
         throw e;
      } else {
         //just log
      }
   }
}

EDIT:

To clarify, MaximumFailuresPolicy is meant to check that atleast one of the component should be initialized successfully. Maybe I have named it incorrectly

Is it a proper way to achieve this? Any help is appreciated.

\$\endgroup\$

closed as off-topic by Emily L., rolfl Aug 5 '14 at 15:17

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Emily L., rolfl
If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

Policy Naming and Behaviour

Your SingleSuccessPolicy name is not a good one. What you actually have here is a MaximumFailuresPolicy.

To effectively create a "real" SingleSuccessPolicy, you'd need to evaluate the final result of your foreach loop. But that is simply impossible in the catch-block.

The problem I see with your code is, that your SingleSuccessPolicy is required to be correctly initialized with new SingleSuccessPolicy(compData.size()-1);. As soon as a differnt SingleSuccessPolicy is passed in, the behaviour is no more the desired one.

You might consider creating the policy inside the GroupComponent constructor. Then you can make sure that the MaximumFailuresPolicy will always behave as a SingleSuccessPolicy, but that would change a whole lot of your structure.
This assumes your compData are immutable after construction, which in turn would IMO warrant a factory / builder pattern instead of what you have here.

What you want to do is yours to decide, but the current way is IMO too unreliable, especially when the ExceptionPolicy depends on what data you are handling. That is not the point of an ExceptionPolicy.

Nitpicks:

Your indentation is not conform to java-standards. Oracle consistently uses an indentation of 4 spaces (or one tab) for blocks.

From method to for-loop, you have 3 spaces, from for-loop to try-catch the same, and from try-catch to statements, you have 2 spaces.

All three alternatives violate conventions, but above all, you're inconsistent!

Additionally I suggest you mark your arguments as final, whereever possible.

\$\endgroup\$
  • \$\begingroup\$ Can you please refer the edit and comment that I have added in code. Actually required behavior is that if any one component succeeds then n will never reach 0 and it will just log and continue. So I guess there wont be the need to check instanceof for that. \$\endgroup\$ – Narendra Pathai Aug 5 '14 at 14:38
  • \$\begingroup\$ I hear you. The problem with creating exception policy in constructor is I will violate OCP. Also then I will have to pass policy type parameter in the constructor to decide as to which policy should be applied. \$\endgroup\$ – Narendra Pathai Aug 5 '14 at 15:16
  • \$\begingroup\$ @NarendraPathai well currently you pass the Policy itself. If you want to have it an OCP way, you could instantiate your policy by using a FactoryPattern that choses an ExceptionPolicy depending on the PolicyType you pass in ;) Though for the problem at hand, this is IMO overkill.. \$\endgroup\$ – Vogel612 Aug 5 '14 at 15:18
  • \$\begingroup\$ If I create policy object in constructor then I am disallowing someone else to inject in a custom policy type. \$\endgroup\$ – Narendra Pathai Aug 5 '14 at 15:21
  • \$\begingroup\$ @NarendraPathai check the edited comment. I just needed a sec. (discussing a different answer of mine right now, too...) \$\endgroup\$ – Vogel612 Aug 5 '14 at 15:22

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