Conversation with #freelords at Wed Apr 25 20:14:38 2012 on ulf82@irc.freenode.net (irc)

(20:56:46) Renn [Renn@78-0-212-220.adsl.net.t-com.hr] entered the room.
(20:56:49) Renn: hey
(20:56:52) ulf82: hi
(20:57:35) ulf82: So, how is the refactoring going? :)
(20:58:39) Renn: it's good, i did everything regarding session and enum, but i've got a few questions
(20:58:48) ulf82: ok, fire away
(20:59:28) Renn: regarding session tests, i test for every exception/enum pair separatly
(21:00:04) Renn: do you do it like that normally? or i could also just count catches for those exceptions in one method and if count is equal to number of enums then test suceeded
(21:00:43) ulf82: what do you test separately exactly?
(21:01:17) Renn: for example, if get from map returns null, it needs to throw an exception
(21:01:24) Renn: so i test it for every enum separately
(21:01:31) Renn: or is for one enum enough?
(21:01:33) ulf82: I think that would be overkill.
(21:01:36) ulf82: I mean
(21:01:45) ulf82: normally, the tests validate your code.
(21:02:02) ulf82: So if you test for all distinct cases, that should be fine enough.
(21:02:03) ulf82: e.g.
(21:02:17) ulf82: the session needs to handle primitives and generic collections properly.
(21:02:28) ulf82: So you should definitely test for one primitive and one generic collection.
(21:02:40) ulf82: in the update() funciton.
(21:02:52) ulf82: But you do not need to do it for every feature.
(21:03:18) Renn: so if it works for one enum, that is enough?
(21:03:27) Renn: for those enums that use collection
(21:03:32) ulf82: at least the exception on null, I would say so.
(21:03:49) ulf82: sorry, wrong answer.
(21:03:50) ulf82: Yes.
(21:04:10) Renn: ok, i didn't know how thorough you want to be so i made it for all cases
(21:04:19) ulf82: If it is not really complicated to extend it to all enums, you can surely test for all once and for all.
(21:05:04) ulf82: but if you know that all enums should behave the same (barring malicious code), one is enough for testing.
(21:05:11) Renn: well, if we test for every enum, then when we add enum that has different value but uses the same principles like some other enum, then we need to add it to tests
(21:05:32) ulf82: you can loop over all enums by Enum.values() or however this function is called.
(21:06:42) Renn: yes, but then on first enum it will throw exception and end the test
(21:07:08) ulf82: unless you play around with this exception object or so....
(21:07:10) ulf82: but ok.
(21:07:13) ulf82: Yes.
(21:07:13) Renn: thats why i said i could add catch there and count how many exceptions i caught, and if number is equal to number of enums it is ok
(21:07:49) Renn: i think it should be enough to test for one kind of exception, but if you think its neccesarry to test for all, i'll do it
(21:08:29) ulf82: I would only test for all enum instances if you can reasonably expect that they behave differently.
(21:08:35) ulf82: from gut feeling.
(21:08:46) Renn: ok
(21:08:59) Renn: next thing
(21:09:02) ulf82: like the update() function
(21:09:04) ulf82: yes?
(21:09:24) Renn: you can't use Set<UUID>.class, it doesn't work
(21:09:52) Renn: so i made (ew HashSet<UUID>(0)).getClass()
(21:10:09) Renn: but that means that it expects hashset and not any other set
(21:10:28) Renn: btw, why do you think we'll use any other form of set except hashset?
(21:10:57) ulf82: unmodifiable sets.
(21:11:04) ulf82: unlikely, though.
(21:11:07) Renn: i think class on Set doesnt work because set is only interface, so you cant instance it
(21:11:12) ulf82: Can you put this issue on the mailing list?
(21:11:26) ulf82: I have no simple idea right now.
(21:12:14) Renn: so you would want to have a class object that could be compared with Set (any)?
(21:12:44) ulf82: if it can be done without excessive magic, I would prefer it.
(21:13:02) Renn: ok, i'll send it
(21:13:12) Renn: next, you can't put null to map
(21:13:35) Renn: it throws nullpointer exception, so return value from get can't be null
(21:14:00) ulf82: ok.
(21:14:03) ulf82: Is this a problem?
(21:14:05) Renn: do you want that IllegalStateException stays there as check for return value?
(21:14:13) Renn: because i removed it
(21:14:23) ulf82: then we should not need it, yes.
(21:14:27) Renn: ok
(21:14:40) ulf82: wait.
(21:14:56) ulf82: if you have a map without the entry, how do you notice this?
(21:15:28) Renn: it returns null if there is no entry
(21:15:37) Renn: thats why we have that other method to check it
(21:15:44) ulf82: ok.
(21:16:03) ulf82: then it should throw the exception
(21:16:06) ulf82: if there is no entry
(21:16:16) ulf82: that is what we want to indicate
(21:16:41) Renn: so you shouldn't call get if there is no entry
(21:16:56) Renn: ok, i'll add exception there
(21:16:56) ulf82: but instead throw the exception, yes.
(21:18:14) Renn: one more thing, just to be sure, as we can't add null to map, should we still throw IllegalArgumentException for when in update object is null, or to let map throw nullpointer exception?
(21:18:21) Renn: imo, illegal arguments fits better
(21:18:25) ulf82: yes.
(21:18:29) ulf82: so just throw it.
(21:18:34) Renn: ok
(21:19:24) Renn: ah yes, one more thing regarding exceptions, is your practice to put everywhere some specific string that describes the situation that happened?
(21:19:31) ulf82: yes.
(21:19:34) Renn: i mean do you put it in exception, or just throw it if its generic?
(21:19:46) ulf82: the exception should come with a descriptive string.
(21:20:25) Renn: do we then test for that string too?
(21:20:29) ulf82: no.
(21:20:58) ulf82: it is nice if you get a human-readable warning if something goes wrong, but this looks like overkill.
(21:21:10) Renn: aha, ok, then i'll change that
(21:21:39) Renn: now, what will be with code in other parts of the game that uses old session?
(21:22:22) ulf82: this code should be updated to work with the new session.
(21:22:35) ulf82: including the tests (but they will probably break anyway without update)
(21:23:03) ulf82: Admittedly, that was missing from the task: Propagate the changes throughout the code
(21:23:29) Renn: what about methods like session.getClientName?
(21:23:51) Renn: how do we get client info about a client when we have only UUID?
(21:24:08) ulf82: good question.
(21:24:13) ulf82: We can retrieve the UUID
(21:24:20) ulf82: and look the name up in the list of clients.
(21:24:39) ulf82: It might, of course, be simpler to just set the client name as a separate property
(21:25:08) ulf82: For now, maybe you can use the first way.
(21:25:27) ulf82: It is a bit inconvenient to use, but we can modify this if it becomes a habit
(21:26:56) ulf82: any other questions?
(21:27:50) Renn: hm, as far as i can see it, client names are only used in addClient(UUID clientId, String clientName) and then stored in old session, or are they stored somewhere else too?
(21:28:06) ulf82: ok, give me a second.
(21:29:15) ulf82: ok.
(21:29:18) Renn: it is stored in ClientListUpdate as map<uuid, name>, but how do i get the instance of it in ClientChatService?
(21:29:38) ulf82: You receive a Map<client_id, client_name> when entering the server.
(21:29:48) ulf82: this is one of the properties stored in the session object.
(21:30:27) ulf82: so you would just retrieve it and look up your favorite name.
(21:30:32) Renn: it used to be, i removed all old data in session before refactoring... i thought we only need what we discussed then in session
(21:30:57) Renn: so i should leave the map for client name still in session?
(21:31:01) ulf82: the equivalent to getClientName() would be something like:
(21:31:24) ulf82: Map<UUID, String> clientNames = session.get(SessionEntry.CLIENT_LIST);
(21:31:29) ulf82: clientNames.get(uuid)
(21:31:41) ulf82: forgot a cast.
(21:32:41) Renn: is CLIENT_LIST the same as CLIENTS_IN_ROOM?
(21:33:01) Renn: or registered_clients?
(21:33:06) ulf82: REGISTERED_CLIENTS
(21:33:09) Renn: because atm both have set in them
(21:33:22) Renn: ok, then i'll remove set and put map for uuid and string (name=
(21:33:47) ulf82: Then there is an error; this is not a Set<UUID>, but a Map<UUID,String>
(21:34:29) Renn: ok, i think that is all that needs to be refactored regarding code outside session, good
(21:34:52) ulf82: then I would just briefly fix the task...
(21:35:18) Renn: ok, i'll send the mail regarding the Set<UUID>.class to the mail list
(21:35:33) ulf82: yes.
(21:36:04) ulf82: is there anything else besides continuing with this task?
(21:36:53) ulf82: with the design, I mean.
(21:36:54) Renn: not atm
(21:36:57) ulf82: ok.
(21:37:31) ulf82: then what remains to be done is two things, unless you find more.
(21:38:04) ulf82: First, I was not really happy with some of the code flow.
(21:38:19) ulf82: So when the client receives an update, say a ClientRegistrationupdate, it
(21:38:24) ulf82: 1. Passes it to all listeners.
(21:38:44) ulf82: 2. gives it as part of it to the UpdateClientInfoService
(21:39:05) ulf82: 3. The UpdateClientInfoService calls the updateSession() method of the Update
(21:39:27) ulf82: Somehow I did not like the idea that what is done can be found in the update itself.
(21:39:47) ulf82: The other issue is that the client listener needs to be made thread-safe.
(21:40:20) ulf82: It must be possible to have a code flow from another flat that attaches a service to listen for updates, and get some global data from the session object without any chance of information getting lost.
(21:40:27) ulf82: not flat, thread
(21:41:07) ulf82: But if you find other things in the code, feel free to add them.
(21:42:18) ulf82: ah yes, 3.: all the new information must be put into the session object (the room id etc.), when the client receives it.
(21:43:26) ulf82: ping
(21:43:48) Renn: pong, i'm here xD
(21:43:52) ulf82: ok.
(21:43:53) Renn: i was writting the mail
(21:43:57) ulf82: ah, ok.
(21:44:40) Renn: so you mean you would send updates as it is done now, but information would be stored in another object, not in the update itself?
(21:45:11) ulf82: well, I partly already reversed my opinion. But first things first.
(21:45:27) ulf82: I somehow did not like the idea of having the code that updates the session in the update.
(21:46:20) ulf82: I see the virtue, though; the update never hands out its internal data.
(21:46:50) ulf82: And the code is usually trivial, just updating the session correctly.
(21:47:06) Renn: i dont know, i kinda think by default if i get some update, i will get it with information i need in it
(21:47:40) ulf82: well, you do. It is just that the update never releases this information.
(21:47:52) ulf82: but directly gives it to the session object.
(21:49:06) Renn: so you don't like that update directly is involved with session?
(21:49:22) ulf82: well, as I said, I see the advantage
(21:49:52) Renn: could we make something like a worker that would process all the updates and act accordingly? then update wouldn't handle the session as well
(21:49:54) ulf82: that you save one getter for each update
(21:50:22) ulf82: Yes, I think I would prefer something in that direction.
(21:50:56) ulf82: Maybe I could suggest my idea, I think I have a coherent image on this by now.
(21:51:01) Renn: ok
(21:51:04) Renn: go ahead
(21:51:44) ulf82: First, we create a marker interface "SessionUpdate" or so. This indicates that the update carries information that will update some of the session data on the client side.
(21:52:20) ulf82: Then, I would rename client.service.UpdateClientInfoService to UpdateSessionService or so, some name that relates to this interface.
(21:52:30) ulf82: This service processes all instances of SessionUpdate.
(21:52:54) ulf82: As a consequence, the SessionUpdate defines how these updates behave consistently.
(21:52:59) ulf82: There are two ways:
(21:53:22) ulf82: a) leave it as now, so the SessionUpdate defines an updateSession() function that updates a Session object
(21:53:35) ulf82: b) implement a get() function that each of the updates has to, uhm, implement.
(21:53:54) ulf82: In the case of (a), the service calls update.updateSession(session)
(21:54:25) ulf82: In the case of (b), the service calls session.update(SessionUpdate.getType or so, SessionUpdate.get())
(21:54:29) ulf82: mmhhh.
(21:54:47) ulf82: For each update, we also need to know the enum instance what global data it updates.
(21:55:06) ulf82: so I would almost tend towards solution (a).
(21:55:29) ulf82: so, what are your thoughts (or questions)?
(21:56:25) Renn: well, in both cases update will need to know what enum instance to handle, so i don't think there is much difference between them
(21:56:48) ulf82: yes, that was my second thought after typing as well.
(21:57:09) Renn: but in b it would need to be stored in it, but in a it wouldn't need to, it would be only in the method updateSession
(21:57:16) Renn: but that is almost nothing
(21:57:43) ulf82: so in (a), the content of updateSession() would always be something like
(21:57:50) Renn: so i think both are ok, but somehow (a) looks better...
(21:57:57) ulf82: session.update(EnumType.XXX, myInternalData);
(21:58:16) ulf82: in (b), we have the functions getData() and getType() or so,
(21:58:18) Renn: update updates something, it's logical... in b) is service updates session with update
(21:58:19) ulf82: and the service does
(21:58:34) ulf82: session.update(update.getType, update.getData());
(21:58:39) Renn: yes
(21:59:01) Renn: i think (a) is cleaner, as you said before, there is no exposing of internal data
(21:59:09) ulf82: (b) does the better separation in terms of work. (a) seems less typing.
(21:59:14) ulf82: yes.
(21:59:19) Renn: updates does the update itself with its own data, service doesn't need to know anything about it, and can't know
(22:00:09) ulf82: That is also why I slowly tend towards (a).
(22:00:32) ulf82: The only thing that I still do not like is that work is done inside the update, but avoiding this comes with additional ugliness.
(22:01:30) Renn: so the conclusion would be? let's take a?:)
(22:01:37) ulf82: yes. Looks like it.
(22:01:47) ulf82: But good to have discussed it. :)
(22:02:08) Renn: yes
(22:02:21) ulf82: So, I think we can summarize a task by now, can't we/
(22:02:22) ulf82: ?
(22:03:35) Renn: yes, but isn't that a part of the same task regarding refactoring?
(22:03:50) ulf82: the one that you do now?
(22:04:08) Renn: because updateSession method of ClientListUpdate is in error atm because of session object
(22:04:35) Renn: so when i would change it to the new one, i would do the update(enum, object)
(22:04:39) ulf82: It does contain some more things, so I would make it a separate task, but dependent on your current task.
(22:04:50) ulf82: but it is rather small.
(22:05:25) Renn: if you mean to add some more session update to other updates that don't have anything to do with session atm then it's fine i think
(22:05:45) ulf82: yes, that would fit in this task in a natural way.
(22:07:07) ulf82: Ok, I'll just try to summarize.
(22:07:08) Renn: ok, because atm, with refactoring i would need to change ClientRegistrationUpdate and ClientListUpdate
(22:07:14) ulf82: yes.
(22:07:17) Renn: i wouldn't try to change any other update
(22:07:22) ulf82: that is fine.
(22:07:25) Renn: ok
(22:07:31) ulf82: So:
(22:07:42) ulf82: 1. Create a new interface network.update.SessionUpdate
(22:07:49) ulf82: another name? or is this fine?
(22:08:00) Renn: i think it's fine
(22:08:02) ulf82: ok.
(22:08:16) ulf82: - it has a function updateSession(Session).
(22:09:03) ulf82: 2. Make the following classes derive from it: ClientListUpdate, ClientRegistrationUpdate, ClientRoomUpdate, ClientsInRoomUpdate, RoomListUpdate.
(22:09:23) ulf82: - they should implement the updateSession() such that they update the session object with the appropriate entry type.
(22:09:31) ulf82: - add entry types if required.
(22:09:57) ulf82: - uhm, also might be a good idea to require that they use unmodifiable collections for the session object.
(22:10:15) ulf82: (the Session object itself does not care, and this way we make sure the client cannot manipulate it)
(22:10:47) ulf82: does this sound reasonable?
(22:11:44) ulf82: ping
(22:11:54) Renn: it does
(22:11:59) ulf82: ok.
(22:12:11) Renn: btw, does that mean that in update it should be removed method updateSession
(22:12:18) ulf82: ah yes.
(22:12:22) ulf82: that is part of 1.
(22:12:31) ulf82: or so.
(22:12:53) Renn: ok
(22:13:34) ulf82: then 3. Rename client.service.UpdateClientInfoService into ...
(22:13:41) ulf82: ClientSessionUpdateService?
(22:14:21) ulf82: - it should accept any SessionUpdate.
(22:14:35) Renn: ok
(22:14:42) ulf82: - and during execution call the updateSession function
(22:14:49) ulf82: dot.
(22:15:14) ulf82: Ah yes, 4. propagate changes (I think FreelordsClientBuilder will fail with these changes)
(22:15:17) Renn: so the same as now, only it will accept different update
(22:15:29) ulf82: and a wider class of updates, yes.
(22:16:07) ulf82: now: do you spot anything amiss? or does anything look undiscussed?
(22:16:46) Renn: not atm, nothing comes to mind
(22:16:53) ulf82: ok.
(22:16:59) Renn: it might later when i do final refactoring, i'll just ask then :)
(22:17:03) ulf82: SoL do you want to write another task?
(22:17:04) ulf82: ;)
(22:17:21) ulf82: (I mean, write this task up)
(22:17:54) Renn: ok
(22:18:04) ulf82: ok. then just say when you are done.
(22:18:22) Renn: that would be new feature?
(22:18:28) Renn: or refactoring?
(22:18:33) ulf82: refactoring
(22:18:37) Renn: ok
(22:20:21) Renn: btw, how do you put deadline to be in like 10 days, but without puting the date? because i see some task have different deadlines, but i can't see any specific date, when i tried to change deadline on my task i could only change date
(22:20:48) ulf82: they are not deadlines, just how long they have been active.
(22:21:30) ulf82: right now, we do not really use deadlines.
(22:21:31) Renn: ahaa, haha, ok
(22:21:36) Renn: ok
(22:22:03) Renn: can task be auto deleted?
(22:22:13) ulf82: what do you mean by this?
(22:22:58) Renn: if it is deleted by itself after a specific number of days passed without it being closed
(22:23:33) ulf82: no.
(22:24:04) Renn: ok, because i think someone asked on mailing list something about task being deleted, don't remember what it was exactly
(22:24:37) ulf82: I accidentally deleted one.
(22:24:47) ulf82: I thought it was left over, but it was just not finished yet.
(22:25:45) Renn: aha, ok
(22:39:24) Renn: i think it's done
(22:39:33) ulf82: ok, having a look...
(22:40:25) ulf82: looks good.
(22:41:13) ulf82: Then, shall we have a look at the thread-safety thing?
(22:41:17) Renn: ok
(22:41:25) ulf82: ok.
(22:41:29) ulf82: So the principal idea:
(22:41:46) ulf82: say, your UI list of clients wants to be up to date under all conditions.
(22:41:52) ulf82: Then, the order would be:
(22:42:07) ulf82: 1. Attach a service that warns you if a new list of clients comes in
(22:42:15) ulf82: 2. get the latest list from the session object.
(22:42:36) ulf82: That way, you avoid having an update accidentally slipping by.
(22:43:10) ulf82: Now we need to make sure that there is proper synchronization to ensure that nothing can happen.
(22:43:44) ulf82: The easiest way would be to go into FLNetworkClientListener
(22:44:01) ulf82: and make addService() removeService() and received synchronized
(22:44:11) ulf82: ok so far?
(22:44:45) Renn: ok
(22:44:56) ulf82: Now the problem here is the potential for a deadlock.
(22:45:17) ulf82: Imagine one of the services calls some update, which ends up with adding another service.
(22:45:26) ulf82: I do not know if this is a problem.
(22:46:37) Renn: hm, not sure either... i think in theory it should be, because it is part of the synced method
(22:47:12) ulf82: right.
(22:47:26) ulf82: So, we somehow need to break this vicious cycle, but how?
(22:48:26) Renn: deadlock would happen because methods for adding/removing are synced, what if we just make the set synced?
(22:49:41) ulf82: you mean the list of services?
(22:50:06) Renn: yes
(22:50:17) ulf82: then you could in principle miss an update.
(22:50:29) ulf82: you attach a new service.
(22:50:40) ulf82: set says "fine, I will do it"
(22:50:52) ulf82: at the same time an update is in the processing
(22:51:05) ulf82: and you retrieve the data before the service updated the session object
(22:51:09) ulf82: brb
(22:52:26) Renn: ah yes
(22:54:19) ulf82: what could be a solution is to share a mutex
(22:54:28) ulf82: between the addServices method ....
(22:54:30) ulf82: well.
(22:54:59) ulf82: In principal we only need to make sure that either the adding is done and respected before the service is updated
(22:55:03) Renn: can you do that without implementing your own implementation of synchronization?
(22:55:09) ulf82: or that the adding is delayed until the session is updated
(22:55:14) ulf82: (session, not service, sorry)
(22:55:52) ulf82: good question.
(22:57:37) ulf82: In principle it is only the execution of the ClientSessionUpdateService that needs to be synchronized.
(22:58:49) ulf82: mmh.
(22:59:29) ulf82: We can of course delay this problem until we run into it.
(22:59:35) Renn: i think doing home-made synchronization would be interesting :) but would need to read more about it
(23:00:01) Renn: i'm searching for books about concurrency in java atm, i'll see what i can find in there
(23:00:03) ulf82: well, probably the solution would be something along: get a mutex and lock it.
(23:00:29) ulf82: which is what synchronized sort of implicitely does on the current object.
(23:01:28) Renn: yes
(23:01:49) ulf82: ok. do we want to delay this to the next chat, then/
(23:01:51) ulf82: ?
(23:02:41) Renn: yes
(23:02:59) ulf82: and you and hopefully I look up the synchronization things.
(23:03:04) ulf82: So: When?
(23:03:09) Renn: yes, i'll try
(23:03:31) ulf82: I should have time this weekend.
(23:03:37) ulf82: Next week might be a bit problematic.
(23:03:41) Renn: hm, i'm away over the weekend, till tuesday, so any time after then?
(23:03:46) ulf82: ok.
(23:03:59) ulf82: We can do Wednesday, then.
(23:04:09) Renn: ok, again at 8?
(23:04:14) ulf82: yes.
(23:04:29) Renn: ok, good, then cya next week
(23:04:32) ulf82: ok.
(23:04:36) ulf82: then have a nice weekend
(23:04:39) ulf82: long weekend.
(23:04:42) Renn: i hope i'll find something usefull till then
(23:04:46) ulf82: ok.
(23:04:49) Renn: ty :)
(23:04:57) ulf82: ok.
(23:04:59) Renn: you too
(23:05:01) Renn: bye
(23:05:03) ulf82: bye
(23:05:17) Renn left the room.