Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abstract rules should be supported through inherited annotation(s) #66

Closed
cogito-clarus opened this issue Mar 19, 2017 · 8 comments
Closed
Assignees
Milestone

Comments

@cogito-clarus
Copy link

cogito-clarus commented Mar 19, 2017

The support of inheriting the boilerplate decorations from a abstract class should be added to reduce said boilerplate in child classes.

  • @inherited should be added to all EasyRules annotations source
  • RuleDefinitionValidator.java/Utils.java in EasyRules lib should check to see if superclass has the valid definition if current class doesn't.

*** EasyRules user's abstract class looks something like ***

@Rule
public abstract class AbstractRule  {

   private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

   private String name;
   private boolean enabled;
   private int priority;

   public AbstractRule() { }

   public AbstractRule(String name, boolean enabled, int priority) {
      this.name = name;
      this.enabled = enabled;
      this.priority = priority;
   }

   @Condition
   public abstract boolean when();

   @Action
   public abstract void then();

   @Priority
   public int getPriority() {
      return this.priority;
  }
}

*** concrete class would look something like ***

 // the rule annotation is from the parent decoration, so not required here
public class oneRule extends AbstractRule {

   private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

   private int input;

   public oneRule(String name, boolean enabled, int priority) {
      super(name, enabled, priority);
      //whatever else
   }
   // the condition annotation is from the parent decoration, so not required here
   public boolean when() {
      return input % 1 == 0;
   }
   // the action annotation is from the parent decoration, so not required here
   public void then() {
      // do something important here
      LOG.debug("isOne => true for {}", input);
   }

   public void setInput(int input) {
      this.input = input;
   }
}
@cogito-clarus cogito-clarus changed the title abstract rules should be supported abstract rules should be supported through inherited annotation Mar 19, 2017
@cogito-clarus cogito-clarus changed the title abstract rules should be supported through inherited annotation abstract rules should be supported through inherited annotation(s) Mar 19, 2017
@fmbenhassine
Copy link
Member

fmbenhassine commented Mar 21, 2017

Hi,

Good idea, you are right. When you want your rules to inherit some common logic from an abstract rule, it is cleaner to put annotations once on the base class and override methods in child classes. I do strongly agree with you on that principle, but the effort to make Easy Rules do it is by far greater than the "boilerplate" it is supposed to save the developer from.

Making Easy Rules introspect the whole object hierarchy to look for condition/action methods in abstract classes is not that easy. It looks easy on an example with one abstract class and two concrete classes. But if you want to implement it, there are many considerations to take into account:

  • How to deal with a deep hierarchies?
  • In your example, how Easy Rules should match methods between child and base class? By name? If we start with "OneRule" class and introspect all its methods, how to detect what method is the condition method? Probalbly we can loop on methods, check if there is a method with the same name in the base class having the @Condition annotation, if not move to the upper level, and so on. This is even more complex for action methods because you can have multiple ones with different execution orders and so on..

You see what I mean? The effort is not that easy, without even considering performance issues (I'm not prematurely opimizing things).

Remember, I agree on the principle, but in practice, the boilerplate of putting annotations on child classes (if we can really call this boilerplate) is not worth the complexity it will introduce in Easy Rules.

Do you agree?

Kind regards
Mahmoud

@cogito-clarus
Copy link
Author

I thank you for taking the time to analyze my humble request and respond in-kind. My goal was to suggest or provide myself, a slight enhancement to your very rational approach to a rules library - keeping things simple, not trying to boil the ocean. I'm buried right now, but I will submit a pull request and examples for you to consider, within the coming weeks.

I think the deep hierarchy issue requires more analysis - I'd love to know the more complicated use cases engaged by library users - my intuition says it's probably not as big of a problem as one might think, especially if you manage expectations up front; a user of the library who cares to decorate an abstract "rule" class gets the responsibility of implementing their solution(s) correctly.

BTW- I'm using your library for an IOT project at a fortune 10 company, and I've noticed that the division of code implementing the when()/then() methods is seldom 50/50 but more like 90/10 or 80/20 in either direction, so that might factor into the deep hierarchy analysis.

Best wishes,
Dave

@fmbenhassine
Copy link
Member

I thank you for taking the time to analyze my humble request and respond in-kind. My goal was to suggest or provide myself, a slight enhancement to your very rational approach to a rules library - keeping things simple, not trying to boil the ocean. I'm buried right now, but I will submit a pull request and examples for you to consider, within the coming weeks.

Thank you, you are welcome. Feel free to suggest any improvement, easy rules is not perfect!

a user of the library who cares to decorate an abstract "rule" class gets the responsibility of implementing their solution(s) correctly.

I work on a couple of open source libraries, and in my little experience, users will abuse your library.
They will not do that even if you clearly state it in documentation, and they will open bugs and complain.
Anyway, please go ahead if you think this is not too much effort. I'm open to contributions.

BTW- I'm using your library for an IOT project at a fortune 10 company

Very happy to hear that! coool 😄 Please let me know if I can help or if you need any support. I'll be glad to help. It would be great if you can share your experience on twitter or in a blog post.

@fmbenhassine
Copy link
Member

Hi @cogito-clarus

I've re-read this thread and looks like I didn't fully understand your request.

I thought Easy Rules should do a lot of work to introspect deep rule objects hierarchies, but I think adding @Inherited to ER annotations should be a good start as you suggested :

@inherited should be added to all EasyRules annotations source

I've added this in version 2.5.0-SNAPSHOT.
The test I added looks similar to your example and it is passing.

Could you please give it a try and tell me if this is ok for you?

Looking forward for your feedback.

Kr
Mahmoud

@fmbenhassine fmbenhassine added this to the v2.5 milestone May 9, 2017
@fmbenhassine fmbenhassine self-assigned this May 9, 2017
@fmbenhassine
Copy link
Member

To use a snapshot version, please add sonatype's repo as described here

@cogito-clarus
Copy link
Author

Thank you, I will test on Sunday and get back to you!

@fmbenhassine
Copy link
Member

Great! take your time.

Many thanks upfront.

@fmbenhassine
Copy link
Member

@cogito-clarus v2.5 is released with this fix.

If the issue persists, do not hesitate to reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants