Re: [Pyatcron-devel-list] New stuff in CVS and some answers to several questions

[Julien Olivier]

On Tue, 2004-04-13 at 17:09, NICOLOVICI Xavier wrote:
> Hi there,
> 
> I've commited some lines of code (Julien and mine) today. Please read the CVS 
> comments to now what have been changed.
> 

Thanks.

> And here some of the comments I have about programming in this project. 
> WARNING, this is comments, not orders, if you think that I'm wrong or 
> misleading everything, feel free to respond ;-)
> 
> So, here we are:
> 
> My comments about PyAtCron programming.
> 
> * Identation.
> Personnaly, I use Emacs to edit source files. The version provided with FC2 
> uses an identation of 4 characters (by default). Now, I do not mind using 
> spaces or tabs, but as our objective is to be integrated into Fedora system 
> config tools, let's use the 4 spaces identation, as it is actually in others 
> system config tools.
> 

OK.

> * Algorythm in Python and coding styles.
> I've found the following code in PyAtCron classes:
> def getName(self):
>         tmpName = self.name
>         if (len (tmpName) == 0):
>           tmpName = self.task.command
>         return(tmpName)
> 
> Python allows us to write "smart" code, and I would prefer to see:
> def getName(self):
>       if self.name:
>               return self.name
>       else:
>               return self.task.command
> Smarter, huh? ;-)
> 

Yup, sorry :)

> Another thing, instead of having two methods to set the value of an object 
> field, use a generic method. It's easier to maintain. Example:
> "def activate(self)" and "def deactivate(self)" should be replaced by "def 
> setActive(self,boolean)"
> 

I thought of that two, but I saw that we had 2 methods for that in the
diagram (activate and deactivate). That said, I think it was I who put
them into the diagram :)

> * Python programming
> Python has some special method names for objects, which makes objects more 
> Python oriented. For example, implementing the __str__(self) method for an 
> object allows to directly pass it to the "print" method. This avoids doing a 
> "print object.getStr()" call. I let you read the Python reference manual, 
> section 3.3 "Special method names".
> 
> * Object oriented programming
> What I like a lot in OO programming is the way how we can modularized a 
> software. That way, having classes interface, it's easy to make changes to 
> the code without impacting the rest of the software.
> For that reason, when a function requires a lot of lines, it might be 
> interesting to think about creating a class to embed this function code. 
> Example, in the mainwin.py module. There is a *huge* bunch of code to simply 
> build the "Next Run" string. What I've done is extract this code from 
> mainwin.py and created a new class, "SyntacticEngine". This class expect a 
> Scheduler object as parameter to its constructor and export a method that 
> returns the NextRun string. Think how it could be easy then to make changes 
> to this code ;-)
> Note: I've found a small bug in this SyntacticEngine code. The 
> getNextRunString method returns never for the following cron command: "*/23 
> 3-8,10-12 */8 */8 sun /bin/false"
> 

About the bug, I'll look at it, but I have to modify the getNextRun
function anyway as it is prone to error the way it is written now.

Now, I have a note too. I have quickly been through the way you
allocated Ids for tasks, and I think it is the wrong way. The way you do
it currently is to give an Id equal to the size of the list each time
you insert a new item in the list. If it works well the next time, the
problem is that, if you insert elements later - after having deleted
list items before - you might have 2 or more items with the same Ids.

An example:

First allocation:

list[0].id=0 list[0].description="a"
list[1].id=1 list[1].description="b"
list[2].id=2 list[2].description="c"

Then, we remove the second one (id=1).

Now we have:

list[0].id=0 list[0].description="a"
list[1].id=2 list[1].description="c"

Then, we add a new item in the list ("d"):

list[0].id=0 list[0].description="a"
list[1].id=2 list[1].description="c"
list[2].id=2 list[2].description="d"

Finally, we want to remove "c" (Id=2), and we end up removing both "c"
and "d" :(

That's why, in my patch, I kept a newtId property whose goal was to
remember the value of the latest Id inserted so that we don't end up
with duplicates.

More over, in the deleteTask (mainwin.py), you do the following:

del(self.list[self.taskId])

In the previous example, that would mean that, to remove "c" (Id=2), you
would do:

del(self.list[2])

which would really delete "d" :(

In conclusion, I think that we really need to use the nextId property
and the getSchedulerById() method that I introduced in my patch. Else,
it could be very buggy... Another solution would be to reach the
Scheduler object directly (without using Ids at all), but the problem is
that I'm not sure it will play well with the way the GtkTree works. In
C, I would simply use the address (a void * pointer) of the object as an
identifier... but I guess such things are not possible in python :)

See you !

-- 
Julien Olivier <address@hidden>