On Mon, Feb 3, 2014 at 11:39 AM, Маркіян Різун <[email protected]> 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
