> On jún. 9, 2016, 4:05 du, 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.
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. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48494/#review136823 ----------------------------------------------------------- On jún. 9, 2016, 3:23 du, Dmytro Sen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48494/ > ----------------------------------------------------------- > > (Updated jún. 9, 2016, 3:23 du) > > > 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 > >
