----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69771/#review212107 -----------------------------------------------------------
I explaned one of my previous comment in more detail. Of course it applies to all of the case statements. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Lines 186-190 (original), 191-195 (patched) <https://reviews.apache.org/r/69771/#comment297721> My main concern is the code block used in every case statement like this: case "mkdir": { Path path = getPath(commandElement, FS_TAG_PATH); mkdir(context, fsConf, nameNodePath, path); break; } I think we should eliminate this and use it like this (of course it means that we cannot declare the same variable multiple times): case "mkdir": Path path = getPath(commandElement, FS_TAG_PATH); mkdir(context, fsConf, nameNodePath, path); break; Or maybe it would be even better to create separate methods and use like this: case "mkdir": doMkdirOperation(...); break; - Andras Salamon On Jan. 17, 2019, 8:03 a.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69771/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2019, 8:03 a.m.) > > > Review request for oozie and Andras Salamon. > > > Repository: oozie-git > > > Description > ------- > > When I read FsActionExecutor.java, I found a not good code in this class. > When judging which logic to use based on commands, should use "switch/case" > replace "if/else": > “if/else” make the code hard to read > “if/else” make the code hard to extend > “if/else” has low efficience > So I suggest using “switch/case” instead. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java > de55793c > > > Diff: https://reviews.apache.org/r/69771/diff/2/ > > > Testing > ------- > > > Thanks, > > Kinga Marton > >
