> On Feb. 10, 2016, 1:04 p.m., jun aoki wrote: > > ambari-web/app/mixins/wizard/assign_master_components.js, line 1077 > > <https://reviews.apache.org/r/43365/diff/2/?file=1238650#file1238650line1077> > > > > I think this is too much coupling. > > The original one already have 3 functionlities in 1 function. > > 1. validate if condition meets > > 2. 1. go to next page > > 3. reset submitButtonClicked > > > > > > and now the patch is adding yet another functionality > > 4. do something if gotoNext is true. > > which I think it gets worse in terms of "deccoupling is good" > > philosophiy. > > > > > > How about starting with splitting reset part. > > ``` > > var resetCondition = function(){ > > self.set('submitbuttonclicked', false); > > } > > ``` > > and Line 1112 (in onSecondary () ) could simply call it.
The variable submitButtonClicked should be set to false for both conditions (if you click Proceed or Cancel on the popup). That is how it ended up in the same callback function. If it was in another function, this is probably what would happen: When you click 'Cancel', you have one callback, which is to set submitButtonClicked to false When you click 'Proceed', you have a callback to route to the next page. However, you still need to set submitButtonClicked to false (another callback?) In terms of design, I think the present callback function with a parameter being passed into it is good, and handles both cases. - Matt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43365/#review118725 ----------------------------------------------------------- On Feb. 9, 2016, 4:04 p.m., Matt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43365/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2016, 4:04 p.m.) > > > Review request for Ambari, Alexander Denissov, Alejandro Fernandez, Aleksandr > Kovalenko, Alexandr Antonenko, bhuvnesh chaudhary, Goutam Tadi, Jaimin Jetly, > jun aoki, Lav Jain, Newton Alex, Oleksandr Diachenko, and Sumit Mohanty. > > > Bugs: AMBARI-14701 > https://issues.apache.org/jira/browse/AMBARI-14701 > > > Repository: ambari > > > Description > ------- > > There was noAction onSecondary (Cancel button). Implemented onSecondary, and > modified the callback for the same. > > > Diffs > ----- > > ambari-web/app/mixins/wizard/assign_master_components.js 7dc267e > > Diff: https://reviews.apache.org/r/43365/diff/ > > > Testing > ------- > > All unit tests passed: > 10384 tests complete (9 seconds) > 121 tests pending > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 43.184 s > [INFO] Finished at: 2016-02-09T10:43:13-08:00 > [INFO] Final Memory: 15M/429M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Matt > >
