On Mon, 2010-09-27 at 21:00 +0300, Vlad Dragu wrote:
> I finished the UML schema for hackit.
> I’m attaching it.
Thanks! Overall it looks much better - congratulations, the difference
is really noticeable! The overall structure looks good.
There is still some work, but don't get depressed by the number of
comments I've made below ;p Those are much more minor than the issues
there was in the past - and experience should quickly show you how to
avoid a good portion of those pitfalls. Good work! : )
1. Should add references to parents instead of duplicating values
(for example Site.owner_id and Site.owner_session should be a
reference to the instantiated objects, not the values
themselves
2. The values stored in the session should not be directly accessed
by the objects (it would be the equivalent of using global
variables) - instead, the GameSession object should instantiate
the objects with the values it retrieves from the session, so
that the objects never need to access the session variables.
3. Should pass objects and not row ids as arguments. For example,
User.set_homebase() should receive a Site object as an argument,
not the id of the site, which should remain internal information
of the corresponding Site() object.
4. Same thing with some of the properties, like Site.owner_id - it
should be a Site.owner property pointing to a User object. When
you don't want to instantiate some of the attributes until they
are accessed, you can have a look at overloading
http://php.net/manual/en/language.oop5.overloading.php or even
setup real properties capabilities on a BasObjecte that all
classes would inherit
http://www.php.net/manual/en/language.oop5.properties.php#98267
5. The Leaderboard probably belongs more as a child of the
GameSession than of the User - it forces you to pass the User
object as an argument every time, but the Leaderboard is not
specific to each user - it's the position in the leaderboard
which is specific to each user
6. Not sure how you intend to use the User.get_*() and User.set_*()
methods - my guess is that those methods are used to retrieve
and store the relevant data in the DB, right? The thing is that
those values are already stored on the object, so for example
you don't need to pass the avg_pr value as an argument to
User.set_avg_pr() - this method can retrieve it from the object
itself. Renaming the methods as User.push_*() and User.pull*()
(to respectively store the value from the object on the DB, and
update the object value from the DB).
7. User.total_sites, User.avg_pr, User.longest_chain and
User.longest_chain_tag belong to the SitesCollection rather than
directly from User - they describe the user's collection of
sites rather than directly the user.
8. The User.get_site_collection() method is redundant: it's already
a property of the object, instantiated by the constructor.
9. User.hack_last_refill, User.hack_last_refill, Site.last_check,
etc. should be DateTime objects rather than strings, it will be
easier to manipulate afterwards
10. User.login() and User.logout() belong to GameSession more than
User - both these actions change the user and will thus require
to instantiate a new User object - which will have to be done
from the GameSession object. You probably will need the
register() method to be on the User() object though - the User
itself would not change, it would just be updated and switch
from an anonymous status to a registered status, and values like
User.email or User.password would be updated.
11. Missing description and relationship for the PlayerStats and
TagList classes
12. Should probably remove the notion of tree (Site.tree_id) and
rework the tables accordingly.
13. The Site.__construct() method should not use numerical id when
it's instantiated by the GameSession - it should receive the
information always accessible from the request, to avoid having
Site logic outside of the Site class, just to get the id. When
the user navigates to a new site, what you get from the request
is the new site URL, not the site_id. You should instantiate the
current site with a URL then - it's then up to the contructor to
find the relevant row in the table.
14. I think you should have an ActionProcessor class, instantiated
by the GameSession, that knows of all possible type of actions
that can be performed within the game and will call the relevant
method on the right object depending on the action. The
GameSession would then be responsible for instantiating the
environment, and the ActionProcessor for modifying this
environment according to the action the user is performing. It
will allow to better isolate responsibilities, and will also
make it easier to log actions, since the ActionProcessor could
then automatically call the ActionLogger with the right
attributes, independently of the action.
I'll stop for now and let you rework this - I'll review the updated
version again to see if I didn't miss anything.
> I also included in the schema the modification to add each guest as an
> anonymous user and separate the hack/scan points from the game
> sessions.
Yup, that's perfect, thank you!
> I’ve used Microsoft Visio to build it and it has a slightly different
> notation convention from the examples I’ve seen in the books. For
> example it adds the “in” word before parameters in function. And also
> it doesn’t have the “array” data type, so I had to use object instead.
Ok - what are the arrays that are marked as objects? I'll check that
it's ok too.
Thanks for everything Vlad.
Xavier.
_______________________________________________
Hackit Bar mailing list - [email protected]
Wiki: http://community.hackit.cx/
List: http://community.hackit.cx/ml/
Forum: http://community.hackit.cx/forum/
Ideas: http://community.hackit.cx/ideas/
IRC: irc://irc.freenode.net/#politis
