> On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Line 66 (original), 91 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068368#file2068368line94> > > > > Can we please apply some kind of object-oriented design pattern, like > > Builder, here? One huge builder with multiple child builders. > > > > I feel that too many `if-then-else` and too long methods are disturbing > > beyond readability / comprehension. > > > > I also would separate out different responsibilities to different, > > maybe nested, classes - and test them separately.
I created a separate SelectBuilder class and test it separately. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java > > Lines 69-78 (original), 68-94 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068369#file2068369line69> > > > > This looks pretty terrible, please extract / restructure for better > > maintainability. Removed this listing of the filter fields from here. - Andras ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review206880 ----------------------------------------------------------- On Aug. 14, 2018, 9:22 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2018, 9:22 a.m.) > > > Review request for oozie, András Piros and Kinga Marton. > > > Repository: oozie-git > > > Description > ------- > > Added lots of new filter fields, also refactored > SLASummaryGetForFilterJPAExecutor class to eliminate FindBugs errors. I'm not > sure about the filter field names. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/OozieClient.java 949b4532 > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > b54161e9 > core/src/main/java/org/apache/oozie/executor/jpa/sla/SelectQuery.java > PRE-CREATION > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java 3982d1e0 > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java > PRE-CREATION > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSelectQuery.java > PRE-CREATION > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java aa633225 > docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227d > webapp/src/main/webapp/console/sla/css/oozie-sla.css d2f2deee > webapp/src/main/webapp/console/sla/js/oozie-sla.js 2ecad228 > webapp/src/main/webapp/console/sla/oozie-sla.html e5bf6275 > > > Diff: https://reviews.apache.org/r/68140/diff/3/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
