> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java > > Lines 345 (patched) > > <https://reviews.apache.org/r/54383/diff/6/?file=1597608#file1597608line345> > > > > Please refactor these nested classes (make them non-nested). The whole > > class is too big with these classes being defined here.
+1 on extracting nested classes, and applying SRP. > On March 24, 2017, 12:24 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java > > Lines 453 (patched) > > <https://reviews.apache.org/r/54383/diff/6/?file=1597608#file1597608line453> > > > > As I can see it, this is not the visitor pattern. > > > > Traditionally, when you use the visitor pattern, the classes that are > > to be visited implement the accept() method which takes the visitor > > implementation as an argument. Then, if the class contains other classes > > that can be visited, it calls the accept() method on the embedded object > > again, effectively passing the visitor implementation down the call stack. > > > > Also, the visitation begins by calling the accept() method on the > > top-most (external) object. > > > > There is a very good, simple example here: > > https://en.wikipedia.org/wiki/Visitor_pattern#Java_example > > > > I think this is just having different implementations, which are hidden > > under an interface (which is good)/ > > > > I suggest doing the following rename: > > - OozieActionVisitor --> OozieActionHandler > > - AbstractOozieActionVisitor --> AbstractOozieActionHandler > > - Killing/Resuming/SuspendingVisitor --> > > Killing/Resuming/SuspendingHandler Visitor pattern can be heavyweight here, agreed. Renaming `*Visitor` to `*Handler` would remove confusion. - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54383/#review169999 ----------------------------------------------------------- On Jan. 5, 2017, 5:34 p.m., Abhishek Bafna wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54383/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2017, 5:34 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2751 > https://issues.apache.org/jira/browse/OOZIE-2751 > > > Repository: oozie-git > > > Description > ------- > > LocalOozieClient is missing methods from OozieClient > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/OozieClient.java 12c80cb > core/src/main/java/org/apache/oozie/BaseEngine.java 50df897 > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java PRE-CREATION > core/src/main/java/org/apache/oozie/LocalOozieClient.java f734f76 > core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java > PRE-CREATION > core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java 32b0cd0 > core/src/main/java/org/apache/oozie/OozieJsonFactory.java PRE-CREATION > core/src/main/java/org/apache/oozie/servlet/V0JobsServlet.java 2c79ef0 > core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java 80c8ec4 > core/src/test/java/org/apache/oozie/TestLocalOozieClientCoord.java 4decd52 > > > Diff: https://reviews.apache.org/r/54383/diff/6/ > > > Testing > ------- > > > Thanks, > > Abhishek Bafna > >
