Schch fix completion percentage #1455
Conversation
| @@ -29,7 +29,7 @@ public void run() { | |||
| } | |||
| TaskContainmentHierarchyFacade facade = createContainmentFacade(); | |||
| for (Task t : facade.getTasksInDocumentOrder()) { | |||
| if (!facade.hasNestedTasks(t)) { | |||
| if (facade.getDepth(t) == 0) { | |||
There was a problem hiding this comment.
Since the algorithm is now going top-down, you can remove this loop completely and just take the root task from the facade.
There was a problem hiding this comment.
Ah, ok. So there is always a single root task no matter of the project structure?
| @@ -43,29 +43,52 @@ public void run(Task task) { | |||
| recalculateSupertaskCompletionPercentageBottomUp(task, facade); | |||
| } | |||
|
|
|||
| private class PropagatedDays { | |||
| public int myCompletedDays; | |||
There was a problem hiding this comment.
Pls make the class static and fields final
| public int myCompletedDays; | ||
| public long myPlannedDays; | ||
|
|
||
| PropagatedDays() { |
There was a problem hiding this comment.
You probably don't need this constructor
| @@ -43,29 +43,52 @@ public void run(Task task) { | |||
| recalculateSupertaskCompletionPercentageBottomUp(task, facade); | |||
| } | |||
|
|
|||
| private class PropagatedDays { | |||
| public int myCompletedDays; | |||
| public long myPlannedDays; | |||
There was a problem hiding this comment.
Why planned is long while completed is int? In the code, completed is like 100x planned so it should be at least wider
| myPlannedDays = plannedDays; | ||
| }; | ||
| } | ||
|
|
||
| private void recalculateSupertaskCompletionPercentageBottomUp(Task task, TaskContainmentHierarchyFacade facade) { |
There was a problem hiding this comment.
If you take the root task at lines 31-35, you can remove this method completely.
There was a problem hiding this comment.
Ok, but then the implementation of void run(Task task) has to be changed too or its invocations need to be replaced by run().
| } | ||
| int completionPercentage = plannedDays == 0 ? 0 : (int) (completedDays / plannedDays); | ||
| task.setCompletionPercentage(completionPercentage); | ||
| } else { | ||
| long nextDuration = task.getDuration().getLength(); |
There was a problem hiding this comment.
I suggest returning new PropagatedDays(nextDuration * task.getCompletionPercentage(), nextDuration) right away and moving accumulating vars into the previous branch of if
There was a problem hiding this comment.
Any particular reason for this?
This way there have to be two return statements, one within each branch of the if-statement which leaves the method without an obvious return statement.
There was a problem hiding this comment.
Just to have less mutable vars. In fact, I would even remove else like below but that doesn't matter too much to me :)
if (nestedTasks.length == 0) {
return new SubtreeCompletion....
}
long completedDays...
// other code from current if > 0 block
| @@ -43,29 +43,52 @@ public void run(Task task) { | |||
| recalculateSupertaskCompletionPercentageBottomUp(task, facade); | |||
| } | |||
|
|
|||
| private class PropagatedDays { | |||
There was a problem hiding this comment.
Let's rename it to SubtreeCompletion ?
| Task supertask_1_0 = taskManager.createTask(); | ||
| Task supertask_1_1 = taskManager.createTask(); | ||
|
|
||
| supertask_0_0.setStart(CalendarFactory.createGanttCalendar(2000, 01, 10)); |
There was a problem hiding this comment.
I recommend using TaskBuilder interface, like this:
Task task_0_0 = taskManager.newTaskBuilder()
.withStartDate(TestSetupHelper.newMonday().getTime())
.withDuration(taskManager.createLength(1))
.withCompletion(100)
.withParent(supertask_0)
.build();
This will make the test code more readable.
Please pay attention that in the code end date is start date + 1.
There was a problem hiding this comment.
Nice. I just used the available code within the existing tests.
| supertask_0.move(supertask); | ||
| supertask_1.move(supertask); | ||
|
|
||
| supertask.move(project); |
There was a problem hiding this comment.
Project has to be moved under taskManager.getRootTask() as well.
| supertask.move(project); | ||
| // | ||
| RecalculateTaskCompletionPercentageAlgorithm alg = taskManager.getAlgorithmCollection().getRecalculateTaskCompletionPercentageAlgorithm(); | ||
| alg.run(project); |
There was a problem hiding this comment.
Maybe let's just run the algorithm without arguments, applying to the whole project?
| } | ||
|
|
||
| public void run(Task task) { | ||
| if (!isEnabled()) { | ||
| return; | ||
| } | ||
| TaskContainmentHierarchyFacade facade = createContainmentFacade(); | ||
| recalculateSupertaskCompletionPercentageBottomUp(task, facade); | ||
| recalculateSupertaskCompletionPercentage(facade.getRootTask(), facade); |
There was a problem hiding this comment.
May I ask you to update the callers of this method so that they called run without arguments? After that this method can be removed. Here is the list of callers:
TaskModelModificationListener.taskProgressChanged(TaskPropertyEvent) (net.sourceforge.ganttproject)
TestTaskCompletionPercentage.testCompletionWithMilestones() (net.sourceforge.ganttproject.test.task)
TaskModelModificationListener.taskAdded(TaskHierarchyEvent) (net.sourceforge.ganttproject)
TestTaskCompletionPercentage.testCompletionIs50WhenAllNestedTasksHalfCompleted() (net.sourceforge.ganttproject.test.task)
TaskModelModificationListener.taskScheduleChanged(TaskScheduleEvent) (net.sourceforge.ganttproject)
TestTaskCompletionPercentage.testCompletionIs0WhenAllNestedTasksNotStarted() (net.sourceforge.ganttproject.test.task)
TaskManagerImpl.projectOpened() (net.sourceforge.ganttproject.task)
TestTaskCompletionPercentage.testCompletionIs100WhenAllNestedTasksCompleted() (net.sourceforge.ganttproject.test.task)
| } | ||
| int completionPercentage = plannedDays == 0 ? 0 : (int) (completedDays / plannedDays); | ||
| task.setCompletionPercentage(completionPercentage); | ||
| } else { | ||
| long nextDuration = task.getDuration().getLength(); |
There was a problem hiding this comment.
Just to have less mutable vars. In fact, I would even remove else like below but that doesn't matter too much to me :)
if (nestedTasks.length == 0) {
return new SubtreeCompletion....
}
long completedDays...
// other code from current if > 0 block
Hi Dmitry,
it looks like the calculation of the completion percentage is inconsistent.
When the depth of a task hierarchy is greater than 1 and there are gaps between tasks the percentage of the tasks at least two levels above a leave are wrong.
super task_0 and super task_1 should have the same percentage like both instances of the nesteted tasks task_0 and task_3.
In addition to this a milestone counts as a day towards completion but has a duration of 0. This also seems to be inconsistent.
The solution is to calculate the completion using the recursively accumulated completed and total duration of all tasks which do not have nested tasks. For a given tasks this has to be done for the complete part of the hierarchy i.e. sub-tree the task is a part of.
This pull request fixes both issues and includes a new test.
Cheers,
Christoph