> On May 14, 2014, 4:26 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 386
> > <https://reviews.apache.org/r/21247/diff/5/?file=581259#file581259line386>
> >
> >     I see this function call (function definition itself is good) as a 
> > potential land mine. For example, moving this function from line 358 to 
> > 356, would cause buildGroupSummary to break and this error will be very 
> > hard to debug. Your comment here is an indication that this needs special 
> > attention.
> >     
> >     Moving this function call into the getTasksForJob(line 398) function 
> > will obviate the need for this comment and will also make the code flow 
> > more straightforward. 
> >     
> >     I agree that this controller code can use some refactoring, but I think 
> > this is a potential land mine and there is nothing to be gained from 
> > calling it outside the normal call path.
> >     
> >     Please feel free to leave a TODO to refactor getTasksForJob.
> 
> David McLaughlin wrote:
>     buildGroupSummary needs $scope.taskSummary to be defined. If I move it to 
> line 398, then every reason for this being 'a land mine' still exists. This 
> is synchronous, imperative code. Why would it be surprising that you can't 
> just move code around without breaking stuff? 
>     
>     Also, it's not going to be hard to debug. The error will say "Cannot call 
> map on undefined" and 
>     
>     TypeError: Cannot read property 'map' of undefined
>         at buildGroupSummary (http://localhost:8081/js/controllers.js:372:47)
>         at new <anonymous> (http://localhost:8081/js/controllers.js:356:5)
>         at invoke (http://localhost:8081/js/angular.js:3697:17)
>         at Object.instantiate (http://localhost:8081/js/angular.js:3708:23)
>         at http://localhost:8081/js/angular.js:6758:28
>         at link (http://localhost:8081/js/angular-route.js:906:26)
>         at nodeLinkFn (http://localhost:8081/js/angular.js:6212:13)
>         at compositeLinkFn (http://localhost:8081/js/angular.js:5622:15)
>         at publicLinkFn (http://localhost:8081/js/angular.js:5527:30)
>         at boundTranscludeFn (http://localhost:8081/js/angular.js:5641:21) 
> <ng-view class="ng-scope">
> 
> Suman Karumuri wrote:
>     The code is breaks the linear control flow for no good reason. I fail to 
> understand the benefit of doing it the way you did it. The fact that you 
> added a comment indicates a code smell. 
>     
>     If it helps, please rewrite getTasksForJob as follows: 
>     
>         $scope.activeTasks = [];
>         
>         getTasksForJob($scope, auroraClient);
>     
>         function getTasksForJob($scope, auroraClient) {
>           var response = auroraClient.getTasks($scope.role, 
> $scope.environment, $scope.job);
>     
>           if (response.error) {
>             $scope.error = 'Error fetching tasks: ' + response.error;
>             return [];
>           }
>     
>           $scope.jobDashboardUrl = 
> getJobDashboardUrl(response.statsUrlPrefix);
>     
>           $scope.taskSummary = 
> taskUtil.summarizeActiveTaskConfigs(response.tasks);
>           buildGroupSummary($scope);
>           
>           var tasks = _.map(response.tasks, function (task) {
>             return summarizeTask(task);
>           });
>     
>           var activeTaskPredicate = function (task) {
>             return task.isActive;
>           };
>     
>           $scope.completedTasks = _.chain(tasks)
>             .reject(activeTaskPredicate)
>             .sortBy(function (task) {
>               return -task.latestActivity; //sort in descending order
>             })
>             .value();
>     
>     
>           $scope.activeTasks = _.chain(tasks)
>             .filter(activeTaskPredicate)
>             .sortBy(function (task) {
>               return task.instanceId;
>             })
>             .value();
>         }
>     
>     This is just a 4 line change and gets rid of the side-effects you don't 
> like. Further refactoring can be performed in future reviews.

It's linear either way, but I'm conceding defeat here. Moved buildGroupSummary 
into that function. 


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42937
-----------------------------------------------------------


On May 14, 2014, 3:46 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 3:46 p.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Bugs: AURORA-378
>     https://issues.apache.org/jira/browse/AURORA-378
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace the button to show/hide configs with a bar when there are multiple. 
> Tidied up the display of the configuration data. 
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 983101277ffbd1c4017b29f4c86e61315f1bcc78 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> dc515ac818a9af36522bb07a125cd92ff9230fa2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> cd78c20bee7891a6cdfd19943a6f7e8a9dce33df 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7c80bd769b7e80bef0822846166959925b1bf7f8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> a79276d2486fc1122a8b084ea0614cdae759f88c 
> 
> Diff: https://reviews.apache.org/r/21247/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Job page with only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
> Showing config for only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
> Job page with multiple config groups
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
> Viewing one of the groups from config bar
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
> Viewing multiple configs at the same time
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
> screenshot of really small group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Reply via email to