Hi Vlad! I've read the changes you've made through the repository changelogs, and I want to thank you for the work you've done - I have a few comments below, but only a few and they are minor issues.
So congratulations! The PHP code is in good shape now - we'll keep improving things over time, but I think we have a solid base. Now for the details - I'll start by answering a few pending questions/comments: > > - 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? > [vlad] i chose to have those objects separate, because it would have > been a conflict with our getter/setter implementation for the object > properties also, for some functions (like the changes since last > session section) i need to do some reads from the database without > instatantiating full object for the elements Alright - I understand. I may come back to discuss that one later on : ) but for now it's good. > When the text is really long (as is the case with some text from the > first mission) the effect is not very good. It just takes too much > time for the whole thing to appear. > So, do you think i should let it in this way for you to see? Or maybe > try something else? Do you have any ideas? I couldn't see it working yet because of the blocking bug on hackit.cx (I'll have a look at the tickets later on), but I think the solution you've found (adapting the time it takes to show the text to the length of the text) is good. Later on, we may want to tune it to make it look more real (for example, randomly adding typos and backspaces, varying the speed, etc.). But this is really not necessary right now : ) > And the same thing with the replies form the user. According to specs, > they should be hidden (like passwords). I tried it and it looks a > little weird. In my opinion it looks better with visible letters. Ah, I think this is a misunderstanding: the fact that answers are hidden doesn't mean that we should hide what the user types - it means that the user doesn't type anything, he just hits "reply", and imagines what he typed. Like in this game: http://www.scoutshonour.com/digital/ > [David] Vlad spent quite some time on unit tests : adapting them, but > also modifying the code itself to be able to run the unit tests. Xav > when you have a little time it would be interesting if you can discuss > with Vlad to see if there's a way to make this less time consuming. Proppy would be better suited for the advices here, but I can try to help - Vlad, can you describe the issues you've encountered? And the comments: - I think I forgot to pay attention to this on the schema file, so if you want we can keep this for the next iteration, but as discussed before, the Missions class needs to be split between a Mission class (that is subclassed by the actual missions classes, such as FirstMission) and a MissionEngine class, that is responsible for instantiating mission objects, saving/restoring them (methods such as save_state()) or calling the right method corresponding to an action (process_action_queue). => this will allow to only expose methods in Mission that can actually be needed by someone scripting a mission, not the underlying magic. It also gives more flexibility, allowing a player to run several missions at the same time for example - The session is not the right place to store mission state - if the session is reset, the player will lose its current mission state. It should be stored in the database. - Some problems with tabs and spaces - see for example http://bazaar.launchpad.net/~vlad-dragu/hackit/alpha2.0/annotate/124/simpletest/test/hackit_user_class.php to see what I mean. You may want to run http://pear.php.net/manual/en/package.php.php-codesniffer.php to ensure compliance with PEAR standards. - Not sure if you did this: when you need to rename a file (for example first_mission.php => FirstMission.php), use "bzr mv first_mission.php FirstMission.php". It allows bazaar to understand what is going on, and keep the file history instead of creating a new file. - Why do you access a GET variable to get the hack results in FirstMission, instead of using an object property? This relates to a part of the codebase that I don't know very well, so I'm unsure. You never trust anything that directly comes from the client, right? - Need to remove the "return true" from mission file - this should be the default, no need to add an extra line - we can just put "return false" when needed. You should also clear out the commented code in FirstMission.php. 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/
