Updated pull request #42
Conversation
| @@ -431,6 +437,17 @@ Behavioral patterns are concerned with algorithms and the assignment of responsi | |||
| **Applicability:** Use the Callback pattern when | |||
| * When some arbitrary synchronous or asynchronous action must be performed after execution of some defined activity. | |||
|
|
|||
| <<<<<<< HEAD | |||
| */ | ||
| public class FilterChain{ | ||
| private ArrayList<Filter> filters = new ArrayList<Filter>(); | ||
| private Target target; |
There was a problem hiding this comment.
Style issue, make this final since it is initialized in constuctor
|
Let me know when you've implemented the changes and I'll review again. |
|
I made changes according to the comments: |
| @@ -497,10 +515,13 @@ The difference is the intent of the patterns. While Proxy controls access to the | |||
| * [Let’s Modify the Objects-First Approach into Design-Patterns-First](http://edu.pecinovsky.cz/papers/2006_ITiCSE_Design_Patterns_First.pdf) | |||
| * [Pattern Languages of Program Design](http://www.amazon.com/Pattern-Languages-Program-Design-Coplien/dp/0201607344/ref=sr_1_1) | |||
| * [Martin Fowler - Event Aggregator](http://martinfowler.com/eaaDev/EventAggregator.html) | |||
| * [TutorialsPoint - Intercepting Filter](http://www.tutorialspoint.com/design_pattern/intercepting_filter_pattern.htm) | |||
| * [Presentation Tier Pattern](http://www.javagyan.com/tutorials/corej2eepatterns/presentation-tier-patterns) | |||
There was a problem hiding this comment.
Should be: Presentation Tier Patterns
|
Nothing major detected, so I'm off to try to run this on my machine. |
|
Ok, there is one showstopper here. The Java source files have no package declaration (should be com.iluwatar) and they have wrong folder structure. Put this declaration to each source file: Let me know when you have done this, and I'll review again. |
|
Fixed folder structure and added package :) |
|
Great! I was able to run it in Eclipse now. |
| }else if(tempout[1] == null){ | ||
| return "INVALID Contact Number!"; | ||
| }else if(tempout[0] == null){ | ||
| return "INVALID Name!"; |
There was a problem hiding this comment.
Don't know if this will violate pattern structure or filters reusability, but I would like to keep related things together.
For example OrderFilter must know that it works with input array index-field "4", and FilterChain must know that value abscence for tempout[4] is "INVALID ORDER".
I see this magic (numbers and messages) go as constants either to Filter interface, or as public static final fields in each implementation.
if (tempout[OrderFilter.INDEX] == null) { return OrderFilter.ERROR_MSG; }
And one step further (although its also can have more problem with pattern consistency):
can we move INDEX and ERROR_MSG to Filter interface as methods, so we can perform polimorfic loop on list of filters instead of if-else chain?
|
Yes, the magic numbers create an itch for me too. @vehpsr could you please work on this? |
|
I certanly could, but only if @joshzambales will give up on this. Another question for me is: would I allow such/additional methods, for example, in chain-of-responsibility pattern implementation?... |
|
Yes, the Chain is certainly similar in implementation. But AFAIK they solve different problems. There is good discussion about this here and here. Also, Intercepting Filter is well know and documented in Core J2EE Patterns. |
|
Created issue #43. @joshzambales @vehpsr let me know if you want to work on it, otherwise I will take it myself. |
|
@all-contributors please add @joshzambales for code |
|
I've put up a pull request to add @joshzambales! 🎉 |
Changelog:
-Split app into separate java files
-Added javadocs
-changed image url to point to iluwatar (UML diagram)
-changed setter injection to constructor injection
-Added intercepting-filter to pom.xml parent as a module
-configured intercepting-filter's pom.xml (my bad, careless mistake)
-separated buttons from button array and changed variable names for more consistent code
Considerations:
To make code more organized and neat, I've decided not to use anonymous classes anymore. Also, if possible, consider retention of FilterManager as I just followed the framework of my source and the delegation is how it manages the FilterChain.