hi Xavier,
I looked at the code review changes.
I estimate about half a day - a day of work. i can do it this Monday but
this means that i will push back the missions on Tuesday.
will that be ok?
vlad
On 10/30/2010 5:45 PM, Xavier Antoviaque wrote:
Hi Vlad,
I've spent some time on the code to familiarize myself with the changes.
There is a lot to go through, so I haven't looked at everything in
details, but I think I've got a good overview of how things are
structured now.
I really like it, I think it's taking a good shape. There is still some
tuning to do (there will always be some), but a good bulk of work is
behind now. Congratz!
Here are some comments/changes/questions - I'll let you read them, let
me know what you think.
===
- Dataobjects auto-generated for Ajaxim: Zajaxim_friends.php& ajaxim
directory => is it remnants from trying to add ajaxim in the past? If so
they need to be removed
- Dataobject files need to be chmod 644
- Missing a merge from lp:hackit
- Same issue as David - https://bugs.launchpad.net/hackit/+bug/668243 .
I've updated a few files on lp:hackit, but it seems that it's missing
the SQLs for the updated database structure (see the ticket)
- Missing an update of the CHANGELOG
- Need to add instructions to regenerate data objects
- Format of the class names: "OneTwoThree", not "one_two_three"
- Why did you separate User and the users data object? You add yourself
a lot of trouble - you could simply add the methods directly on the
dataobject, and expose it directly, no? But maybe there is something I
didn't see. (And same question about the Site and GameSession classes,
actually)
- Instead of individual arguments about the current site directly
attached on GameSession, should attach a Site as
GameSession->current_site, containing:
* public $web_address;
* public $encoded_address;
* public $proxy_page;
* public $hack_probability;
* public $scan_probability;
- Need to remove the part "TO DO remove at end of 2.0 iteration" ;p
- You could move more code to the dataobjects themselves - basically
code that only require to manipulate dataobjects themselves should be
considered, especially if it could be useful to different objects.
- I missed that one in the UML schema: code related to hacks and scans
should be isolated on other objects, here it makes the game session
class HUGE. Why didn't you put it in the action observers? You could
actually isolate a good portion of the GameSession logic into smaller
dedicated objects this way.
- Btw, you should put all the files containing the classes for the
action observers in a dedicated subdirectory of classes/ . And congratz
btw for the mechanism here, it works very gracefully.
- prepare_address() should be on the Site class
Xavier.
_______________________________________________
Farsides mailing list - [email protected]
Wiki: http://farsides.com/
List: http://farsides.com/ml/
Forum: http://farsides.com/forum/
Ideas: http://farsides.com/ideas/
Chat: http://farsides.com/chat/
_______________________________________________
Farsides mailing list - [email protected]
Wiki: http://farsides.com/
List: http://farsides.com/ml/
Forum: http://farsides.com/forum/
Ideas: http://farsides.com/ideas/
Chat: http://farsides.com/chat/
