Code review comment for lp:~nimit-svnit/gtg/dates

Hi Bertrand!
   Thank you for the review!

On Thu, Jun 7, 2012 at 4:03 AM, Bertrand Rousseau <
<email address hidden>> wrote:

> Review: Needs Fixing code inspection
>
> Some remarks:
>
> (line numbers are references from your patch)
>
> - I suggest you move line 36 in top of the method code, and remove the
> line 22. It seems to me that the current code will update the due_date two
> times if the start date comes after the new due date
>

You are right. I had made a small modification earlier. I should have moved
the line up after I removed the modification.

> - in line 27, you seem to test if the parent has a due date by doing "if
> par.due_date", shouldn't it be "if par.due_date != Date.no_date()"?
> - same remark for the children start date in line 34 ("if sub.start_date")
>

Regarding the par.due_date != Date.no_date(), I just tried "if
par.due_date" and it was working for me, hence I thought of keeping it that
way. Anyways, I will modify it as that will be better to understand.

> - you should use the accessor functions when you need to get the
> start/due date from other object than the current (parents and children
> tasks)
> - if you unset the due date (fulldate == Date.no_date()), shouldn't you
> skip all parent/children updates? Right now, you skip only the parent update
>

fulldate == Date.no_date check was required because of the comparisons of
the dates. If I simply kept the comparison "par.due_date < Date(fulldate)"
then when I would clear the due_date of child, the due_date of parent would
also be cleared. Hence I needed to keep this check.

 - you shouldn't change the due date accessor, it seems to me that the
> previous behavior was correct.
>

I'll explain the reason I removed the due_date accessor and was not using
get_due_date.
     Earlier I was using the due_date accessor and the old implementation.
But the problem was, get_due_date returns the most urgent due_date amongst
it's parents but doesnot set the child's due_date equal to it. So, after
setting the parent's due_date, when I ran the loop for child. The loop
would run uselessly because, the get_due_date would return a due_date same
as that of parent (in my case). Hence, even though the actual due_date of
child would be later, the get_due_date accessor's behaviour made it
impossible to update it without directly accessing the date.

    If we use the my modified get_due_date then the real due_date would be
returned which will make it possible to view actually modify the child.

> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785<https://code.launchpad.net/%7Enimit-svnit/gtg/dates/+merge/108785>
> You are the owner of lp:~nimit-svnit/gtg/dates.
>

Nimit Shah,
B Tech 3rd year,
Computer Engineering Department,
SVNIT Surat
Secretary ACM-NIT Surat
www.dude-says.blogspot.com