> 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">
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.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42937
-----------------------------------------------------------
On May 14, 2014, 1:30 a.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 1:30 a.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
>
>
