----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69771/#review212056 -----------------------------------------------------------
I really like the idea of cleaning up this code, using switch is much better than the old if/else. I suggest a few more things to make the code even cleaner. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Lines 186 (patched) <https://reviews.apache.org/r/69771/#comment297655> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 186 (original), 187 (patched) <https://reviews.apache.org/r/69771/#comment297665> This String literal is used several times. I would extract it to a constant. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 189 (original), 191 (patched) <https://reviews.apache.org/r/69771/#comment297656> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 193 (original), 194 (patched) <https://reviews.apache.org/r/69771/#comment297666> Please extract commandElement.getAttributeValue("skip-trash") into a variable. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 199 (original), 201 (patched) <https://reviews.apache.org/r/69771/#comment297657> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 205 (original), 207 (patched) <https://reviews.apache.org/r/69771/#comment297658> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 209 (original), 210 (patched) <https://reviews.apache.org/r/69771/#comment297669> Please choose a better variable name. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 214 (original), 216 (patched) <https://reviews.apache.org/r/69771/#comment297659> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 219 (original), 221 (patched) <https://reviews.apache.org/r/69771/#comment297660> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Lines 222-223 (original), 223-224 (patched) <https://reviews.apache.org/r/69771/#comment297662> Do we still need to break the line to keep the length <= 132 characters? Thanks to your cleanup we have much less indentation here. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 225 (original), 226 (patched) <https://reviews.apache.org/r/69771/#comment297670> Please find a better variable name. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Lines 227-228 (original), 228-229 (patched) <https://reviews.apache.org/r/69771/#comment297663> Do we still need to break the line to keep the length <= 132 characters? Thanks to your cleanup we have much less indentation here. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 230 (original), 232 (patched) <https://reviews.apache.org/r/69771/#comment297661> Please remove the { } code block. If necessary declare the variables before the switch. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Lines 233-234 (original), 234-235 (patched) <https://reviews.apache.org/r/69771/#comment297664> Do we still need to break the line to keep the length <= 132 characters? Thanks to your cleanup we have much less indentation here. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 235 (original), 236 (patched) <https://reviews.apache.org/r/69771/#comment297667> We could use replicationFactor variable here core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 239 (original), 241 (patched) <https://reviews.apache.org/r/69771/#comment297654> What happens if command is not found? I would add a default section with a warning. core/src/main/java/org/apache/oozie/action/hadoop/FsActionExecutor.java Line 241 (original), 243 (patched) <https://reviews.apache.org/r/69771/#comment297653> Please add linebreak, as suggested here: https://cwiki.apache.org/confluence/display/OOZIE/How+To+Contribute - Andras Salamon On Jan. 16, 2019, 8:23 a.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69771/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2019, 8:23 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/1/ > > > Testing > ------- > > > Thanks, > > Kinga Marton > >
