> On Feb. 2, 2017, 4:33 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java, > > lines 406-429 > > <https://reviews.apache.org/r/56158/diff/3/?file=1621110#file1621110line406> > > > > This `static` method I'd extract to a - maybe nested - class called > > `StartCalendarFinder` and use constructor parameters `Date nominalTime, > > Date initialTime, TimeZone timeZone, int startIndex, TimeUnit timeUnit`, > > and one sole `public Calendar findStartCalendar()` method. > > Satish Saley wrote: > Could you please mention any specific reasons for doing it that way? What > advantages will it bring? > > András Piros wrote: > Thanks for asking! > > The approach I suggested is better for code readability (Single > Responsibility Principle - `CoordCommandUtils` does way too much also in the > moment) and testability. > > I recommend reading Misko Hevery's testability blog, here is [the > article](http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/) > why we should use as little `static` methods as possible
I went through the article. It looks good. But I have +1 on current patch from Rohini. Let me check if she has cycles to review this change. - Satish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56158/#review164056 ----------------------------------------------------------- On Feb. 5, 2017, 1 a.m., Satish Saley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56158/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2017, 1 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2630 > https://issues.apache.org/jira/browse/OOZIE-2630 > > > Repository: oozie-git > > > Description > ------- > > [OOZIE-2630] Amend patch for OOZIE-2630 > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java > 3a7a930 > core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java > 22d1f61 > core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00 > core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac > core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4 > core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION > docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9 > > Diff: https://reviews.apache.org/r/56158/diff/ > > > Testing > ------- > > Tested locally > > > Thanks, > > Satish Saley > >
