Thanks damien It was on my todo :) Mark will come to visit us this summer :)
Stef On 04 Feb 2014, at 17:37, Damien Cassou <damien.cas...@gmail.com> wrote: > > On Mon, Feb 3, 2014 at 11:39 AM, Маркіян Різун <mri...@gmail.com> wrote: > As a novice in pharo, waiting for comments and advice. > Сritique is welcome too ;) > > random comments (tested on Pharo 3.0): > > - I played and things work perfectly as far as I saw :-) > - your code is of very good quality! > - 2 tests don't pass (EmptyCellTestCase, CellRendererTestCase) > - usually we don't append 'Case' at the end of our test classes > - the package name doesn't follow the convention > (https://ci.inria.fr/pharo-contribution/job/PharoForTheEnterprise/ws/Conventions/Conventions.pier.html) > - you may want to avoid the use of non-alphanumeric characters in the package > name (e.g., avoid &) > - please indent your code (use ALT+Shift+u on each method to get automatic > indentation) > - "self should: [ offset = (50 @ 50) ]" --> "self assert: offset equals: > 50@50" > - "self should: [ cell isOff ]." --> "self assert: cell isOff" > - all subclasses of Cell override #initialize in exactly the same way. > Consider moving that to the superclass > - you should try to avoid as much as possible doing a super send in a > different method. For example, EmptyCell>>initializeState does a super send > on #initialize. > - OCell and XCell are exactly the same, this should not be and is very > suspicious. > - CellRenderer has 2 methods with an uppercase letter. This is not > conventional > - you use a lot of variables you use only once. Consider inlining them. For > example in CellRenderer>>offsetWithinGridForm, all variables seem useless : > "^ CellRenderer cellExtent * ((self cellLocation x - 1) @ (self cellLocation > y - 1))" > - CellRenderer>>renderContents uses branching on the class of objects instead > of polymorphism. That's bad design in my opinion. Consider delegating to > OCell and XCell or using double-dispatch. > - Same thing for CellRenderer class>>rendererFor: > > All in all, very good job. Would you consider writing a tutorial on how to > implement an Xs&Os game in Pharo? > > -- > Damien Cassou > http://damiencassou.seasidehosting.st > > "Success is the ability to go from one failure to another without losing > enthusiasm." > Winston Churchill
