> On Июнь 9, 2016, 4:05 п.п., Daniel Gergely wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java, > > line 343 > > <https://reviews.apache.org/r/48494/diff/1/?file=1412628#file1412628line343> > > > > Please consider cyclomatic complexity in this method. > > Dmytro Sen wrote: > What is wrong with it ? The loop body is executed only once for every > property. > Moreover, this method is executed only once during ambari server > lifecycle, when the server is least loaded. > > Daniel Gergely wrote: > Loops have no relations with cyclomatic complexity. I think you wrote > about computational complexity (more specifically time complexity). > Cyclomatic complexity is a measure to show how many possible execution paths > are there in a specific block (eg in a method body or in a class). Each of > the **if** statements increases this number, since there are always 2 > options: true or false. > I counted 11 levels of deepness here, most of them are if statements, > which means a high cyclomatic complexity. > My review was only about avoiding it, because it makes the code less > maintainable and readable. Computational complexity won't be less, business > logic won't be changed, what you gain is maintainibility and in some cases it > can be easier to test. > There are many ways how to reduce it, usually some code can be simplified > or a new method can be introduced. (if you google it up, you will find many > examples. First I would extract methods.) > This is only suggestion, to avoid bad moments for the developer who may > need to change the code in the future.
Thanks for detailed clarification. I'll update the patch. - Dmytro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48494/#review136823 ----------------------------------------------------------- On Июнь 9, 2016, 3:23 п.п., Dmytro Sen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48494/ > ----------------------------------------------------------- > > (Updated Июнь 9, 2016, 3:23 п.п.) > > > Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi > Brodetskyi. > > > Bugs: AMBARI-17146 > https://issues.apache.org/jira/browse/AMBARI-17146 > > > Repository: ambari > > > Description > ------- > > Implement config values trimming for deployment via blueprint as we do in UI > > trimProperty: function (property) { > var displayType = Em.get(property, 'displayType'); > var value = Em.get(property, 'value'); > var name = Em.get(property, 'name'); > var rez; > switch (displayType) { > case 'directories': > case 'directory': > rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(','); > break; > case 'host': > rez = value.trim(); > break; > case 'password': > break; > default: > if (name == 'javax.jdo.option.ConnectionURL' || name == > 'oozie.service.JPAService.jdbc.url') { > rez = value.trim(); > } > rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : > value; > } > return ((rez == '') || (rez == undefined)) ? value : rez; > }, > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java > de70a2c > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java > ad8d4f9 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java > 9ec0a09 > > Diff: https://reviews.apache.org/r/48494/diff/ > > > Testing > ------- > > Unit tests and manual tests passed > > > Thanks, > > Dmytro Sen > >
