[kplato] Some code for KPTProject::getDuration()

[John D Lamb]

Thomas Zander wrote:
> 
> > I'm not yet sure what the purpose of KPTRelation is. This may be
> > ignorance on my part. What is clear is that it encapsulates the idea
> > that (say) Node A comes before Node B in a Project. My instinct would be
> > to encapsulate this by listing B as a time-dependent child of A and A as
> > a parent of B. I think any other information can be calculated
> > efficiently or encapsulated by a node ... of course if I've got this
> > wrong, its much easier to work with a KPTRelation than without it. ;-)
> 
> The current solve is correct, take a step back and see it like this;
> 
>         X --- (lag=10) --- Y
> 
> item X has a relation with item Y, but there is a dependency (call it
> direction or in this case lag) in the relation.

I thought there might be something like that there. This would be
difficult to implement by additional dummy nodes and won't (I think)
cause any problems for the PERT/CPM calculations. Also the KPTRelation
structure essentially defines arcs between nodes -  a convenient place
to put private temporary data during PERT/CPM :-)

> Then the data of that relation should either be stored in the relation
> itself,  or in _both_ the item X and item Y.
> 
> I have chosen for the latter since we dont want duplication info.
> 
> Other then that; it is possible to add more then one dependency between
> 2 nodes, and at the moment you create 2 lists which should be in sync with
> each other (one list for the child-nodes and one for their relation) you
> should look if it is not wiser to create a class to combine that data.

That sounds right, but the code at the moment has 3 lists (I think this
confused me). m_nodes makes obvious sense, but why not just one list:
m_relations instead of m_dependChildNodes and m_dependParentNodes. OK,
at the moment I assume m_dependChildNodes should list the Relations
whose parent is this and whose child is another node with the same
parent as this. But that seems to have duplication.

Here's another way that makes more sense to me. If P is a node that is
not a task, then m_nodes contains all its subprojects and tasks. The
relations ought to belong to P and not to its subnodes. So why not make
a list<KPTRelation*> (or QList<KPTRelation> - more later) in P of all
the KPTRelations between subnodes? As I understand there could not be
any other kind of time dependency. Then in each KPTNode in m_node, we
can calculate the time dependencies at time of need by code something
like the following:

#include<iostream>
#include<list>
#include<algorithm>
#include<iterator>
#include<functional>

using namespace std;

...

class KPTNode ...

  KPTRelation* getFirstDependChild() {
    if( m_parent == 0 )
      return 0;
    else {
      q = find_if( m_parent->m_relations.begin(),
                   m_parent->m_relations.end(),
                   is_successor( this ) );
      return *q;
    }
  }
  Relation* getNextDependChild() {
    if( m_parent == 0 || q == m_parent->m_relations.end() )
      return 0;
    else {
      q = find_if( ++q, m_parent->m_relations.end(),
                   is_successor( this ) );
      return *q;
    }
  }
...
private:
  list<KPTRelation*> m_relations;
  struct is_successor { // This makes is_successor( KPTNode* n ) behave
like a function pointer
    explicit is_successor( KPTNode* n ) : n(n) {}
    bool operator()( const KPTRelation* r ) const {
      return r->s == n;
    }
  private:
    const KPTNode* n;
  };
...
+ similar functions for getting the number of children, the nth child
and the parent versions.
Here I think list<KPTRelation*> would be better than QList<Relation>
because m_relations would be private and STL offers better support for
the internal manipulations: I'm happy to write all the actual functions
for this.

> 
> > I've thought of a sensible way to prevent unnecessary recalculations of
> > durations, floats, earliest starts, etc. Give each node a flag called
> > recalculate. Define in KPTNode a function
> > void setRecalculate(){
> >   recalculate = true;
> >   for_each( child )
> >     setRecalculate();
> > }
> 
> This has as disadvantage that when a node changes a value which would
> cause a recalculation you have to find the project it belongs to, and
> recalculate far to much. (since lots of values don't change and can be used
> again)
> 
> What about the opposite; where a node stores if it should be recalculated
> and a parent node asks all its children if anyone has a recalculate flag
> set.
> 
> This keeps the 'changed-bool' data local to the change and thus prevents extra
> calculations.
Both are possible. I still think mine would make PERT/CPM more
efficient, but in any case you've made me spot a mistake in my logic. If
a node is flagged to be recalculated, then all _future_ (i.e.
time-dependent rather than tree-dependent) nodes have to be
recalculated. 

> > How about a flags userStarted and userFinished on each Node to say that
> > whether the user has specified a start or finish time? with a check on
> > dependent parent nodes before allowing the user to set something. Then
> > the start time, end time and duration are either user fixed or estimated
> > by PERT/CPM. If the user has specified anything, that gives an earliest
> > starttime that PERT/CPM can use. Otherwise, use 0?
> 
> Excelent, and we can leave the flag out since checking if the m_startTime
> (=userStarted) is zero means it is not set.
> 
> Oh, wait, that was my initial design as well ;)
> 
Sounds good. Do we need to cover the possibility that a user has started
an activity, but that it hasn't finished yet?
> > I suggest we add (in a struct?) earliest_start, latest_finish as
> > QDateTimes.
> 
> Just a comment; _dont use structs_. C++ has classes for that :)

I used to avoid them, but in C++ they are classes in which everything is
public by default. Just occsasionally that's useful. There is another
reason to use structs occasionally, but it's so obscure almost no-one
uses it. On the other hand, you may prefer to avoid structs.
 
> These are the calculated values, right? Yes these should be added,
> just as members of the kptNode class seems fine to me though.
> 
> > I thinks that would be enough. Also, rather than having
> > separate functions for expected, random, optimistic, etc. times, why not
> > create another KPTNode enumerated type (or even a set of empty classes
> > if you want to be smug and gain efficiency by getting all the actual
> > functions resolved at compile time) something like
> > enum calcType { expected, random, optimistic };
> > Then define (say)
> > ... getDuration( KPTNode::calcType CalcType = KPTNode::expected );
> > and get the functions themselves to work out what to do. This is likely
> > to be especially useful at say Project level:
> > getDuration(KPTNode::calcType CalcType = KPTNode::expected ){
> >   ...
> >  for_each( child )
> >     child->getDuration( CalcType );
> >  ...
> > }
> > Will that cause more problems than it solves?
> 
> I think this is a nice addition, and basically I like it, but not
> as a replacement.
> So I think we should use operator overloading to create an extra method
> which does the above, but let the original methods exist.
>
That had occurred to me too..
> 
> Then you have the choice of implementing the logic in that one method
> and use the others as 'shortcuts' or the other way around, where the
> 3 methods implement the logic each for the different duration time.
> 
> > Any thoughts?
> > JDL
> 
> Just that it is too hot to do anything intelligent here... (Holland/Europe)
Ja! Het is zeer heet hier ook... (Kent/Europe)
JDL