Updated pull request#42
Conversation
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: |
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. |
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.