Hi benjamin Thanks for your code!!!! I did a review of your latest code: Here are some points that it would be nice to address I fixed what I saw and republished 0 - all the classes should have comments MessageList ---------- 1- MessageList example does not work Can you create some Unit tests to cover the expected behavior of the MessageList 2- MessageList>>browsers I removed it 3- recategorized the methods 4- I removed all the "comment10" 5- what I level Can you comment it? 6- for asDictionaryByClass why don't you use groupedBy: or similar methods? I'm not sure that I would call them asDictionaryBy.... since you return an association? Why don't you call them groupedBySelectors, groupedByClasses, groupedByPackages then why do you return an Association and not a dictionary. 7- Can you add oneline method comments to method? 8- Could you introduce an explicit instance variable representing the environment in use and initialize it with Smalltalk globals because like that we can specify in which environment the set is working. 9- openInWorldWithADictionarySelector: sucks :) the name is not really cool then you bind the core with the UI so you should define this method as an extension of RecentSubmission-UI and may be 10- You update all the browsers open but this should not be your role. As a core domain object you should not do this kind of behavior. The UI widget should register to event or notification and update themselves when you change. updateView browsers ifNotNilDo: [:collection | collection do: [:each | each updateView]] MessageListAssociation -------------------- why do you need this guy? Just to print the elements? This is a UI concerns. The UI should wrap the model and provide a specific printed version or the MessageListAssociation should be explained in the class comment MethodReferenceWithSource ------------------------ We should probably move some behavior to MethodReference NullTextStyler ------------ I categorized all the methods following SHStyler categories RecentMessageList ---------------- I cleaned the singleton part. You do not need UniqueInstance ifNotNil: [:u | UniqueInstance releaseAll]. Again the model should not be referencing the ui releaseAll super initialize. MessageListBrowser allInstancesDo: [:each | each messageList = self ifTrue:[each delete]]. browser:= nil. self methodReferenceList: OrderedCollection new. Why the setting is registration is done in the core and not at the level of the widget? - at startUp: you should not create the instance since uniqueInstance is lazzily accessed. However you should register to the SystemChangeNotifier to get the notification. MessageListBrowser ----------------- Instead of having MessageListBrowser >>byDateAscendingOn: aMessageList ^self on: aMessageList withADictionarySelector: #asDictionaryByDateAscending. MessageListBrowser >>byDateAscendingOn: aMessageList ^self on: aMessageList withADictionarySelector: aMessageList asDictionaryByDateAscendingSelector and MessageList>> asDictionaryByDateAscendingSelector ^ #asDictionaryByDateAscending => encapsulate information from model to UI. => I did it So for now this is enough :) Stef On Aug 1, 2010, at 3:00 AM, Benjamin Van Ryseghem wrote:
Gofer new squeaksource: 'PharoTaskForces'; package: 'RecentSubmissions'; load.