Hi mark

I read a bit your code

It is nice for a first project!!!!

Here are some feedback. I suggest that you take into account the easy one and 
produce a new version and if you want to 
train your skills you try to see how to address the difficult ones.

Stef



        - Add it in the class comment
                XsAndOsGame new openInWorld.

        - you have some unclassified protocols on the class side.

        - you may want to run smallLint (criticBorwser on your code and see the 
hints that it can give you).

        - When you create a new game do not move the window to the top corner :)

        easyBtn -> easyButton (you do not pay per character :))

        findEmptyCellPos -> findEmptyCellPosition
        not in accessing 

        impBtn -> impossibleButton

        …. :)
        
        - Why do you need a separate class just for three colors? 
        I would merge 
                XsAndOsGameColors and XsAndOsGame

        XsAndOsGame>>initialize is long
        May be you should extract some methods from it (methods that you can 
then reuse for other tasks)


renderOContents
        | delta offset x1 y1 x2 y2 rect |
        delta := CellRenderer cellExtent - 1.
        offset := self offsetWithinGridForm.
        x1 := offset x + self oCorner x.
        x2 := offset x + delta x - self oCorner x.
        y1 := offset y + self oCorner y.
        y2 := offset y + delta y - self oCorner y.
        rect := Rectangle new setOrigin: x1 @ y1 corner: x2 @ y2.
        self targetForm getCanvas
                fillOval: rect
                color: XsAndOsGameColors gameBoardBackgroundColor
                borderWidth: 3
                borderColor: XsAndOsGameColors OColor

What do you refer to CellRenderer direcly? self class ?


rendererFor: aCell
        "VERY BAD IMPLEMENTATION OF THIS METHOD!!!"

        aCell class = EmptyCell
                ifTrue: [ ^ CellRenderer ].
        aCell class = XCell
                ifTrue: [ ^ XCellRenderer ].
        aCell class = OCell
                ifTrue: [ ^ OCellRenderer ]

indeed :)
why not implementing some methods call renderClass and sending a message to the 
object :)

mptyCell>>initialize
        super initialize.



CellRenderer>>offsetWithinGridForm
        "Returns a point - left top corner of current cell"

        ^ CellRenderer cellExtent * ((self cellLocation x - 1) @ (self 
cellLocation y - 1))
=> self clas cellExtent

CellRenderer >> renderBorderBottom
        | delta offset x1 y1 x2 y2 |
        delta := CellRenderer cellExtent - 1.
        offset := self offsetWithinGridForm.
        x1 := offset x.
        x2 := x1 + delta x.
        y1 := offset y + delta y.
        y2 := y1.
        self targetForm getCanvas
                line: x1 @ y1
                to: x2 @ y2
                width: 1
                color: XsAndOsGameColors cellBorderColor

=>
CellRenderer >> renderBorderBottom
        | delta offset x1 y1 x2 y2 |
        delta := self class cellExtent - 1.
        offset := self offsetWithinGridForm.
        x1 := offset x.
        x2 := x1 + delta x.
        y1 := offset y + delta y.
        y2 := y1.
        self targetForm getCanvas
                line: x1 @ y1
                to: x2 @ y2
                width: 1
                color: XsAndOsGameColors cellBorderColor


I was wondering why you need Grid (I would rename it CellGrid) and do not use a 
Matrix.

        grid sizeOfGrid
                -> I would rename sizeOfGrid into size like that your grid can 
be a bit more polymorphic from its API point of view


makeGameBoarMorph
        -> typo :)
        
Now I want wondering why you do not have an instance variable to hold it 


Future fun ideas for new versions:
——————————————————————
playerVsComputer
        self vsComputer
                ifTrue: [ 
                        self vsComputer: false.
                        self stateMorph contents: 'Player 1 VS Player 2' ]
                ifFalse: [ 
                        self vsComputer: true.
                        self stateMorph contents: 'Player VS Computer ---> 
Difficulty: ' , self difficulty asString ].
        self vsComputer
                ifTrue: [ 
                        self easyBtn enable.
                        self normalBtn enable.
                        self impBtn enable ]
                ifFalse: [ 
                        self easyBtn disable.
                        self normalBtn disable.
                        self impBtn disable ]
        
This kind of code may be think that may be you need another object that 
represent the 
        TwoUsers
        vsComputer
                strategy
        
        I’m not sure but clicked look quite long. 
        May be you should create another class that can represent the logic of 
the game.

A really interesting exercise would be to see how you can create a boardGame 
for 15*15 and see how it breaks some hidden rules.
On 08 Feb 2014, at 15:07, Маркіян Різун <mri...@gmail.com> wrote:

> Updated XsAndOsGame: added AI, also fixed some problems, that Damien 
> mentioned in his comments.
> Btw, thanks to Damien, because I learned for myself a lot of new useful 
> things.
> Note: The new version is XsAndOsGame (NOT Xs&OsGame) !
> To play it type in workspace: XsAndOsGame new openInWorld.
> If you want to play vs computer press button "ToggleMode" and choose 
> difficulty. Initially it is easy.
> 
> Mark
> 
> 
> 2014-02-05 15:57 GMT+02:00 Маркіян Різун <mri...@gmail.com>:
> Thanks, I'll certainly take a look at it
> 
> 
> 2014-02-05 Damien Cassou <damien.cas...@gmail.com>:
> 
> On Tue, Feb 4, 2014 at 8:58 PM, Маркіян Різун <mri...@gmail.com> wrote:
> > I should think how to write this tutorial.
> 
> 
> on the technical point of view, I advise you to use Pillar:
> http://www.smalltalkhub.com/#!/~Pier/Pillar
> 
> --
> Damien Cassou
> http://damiencassou.seasidehosting.st
> 
> "Success is the ability to go from one failure to another without
> losing enthusiasm."
> Winston Churchill
> 
> 
> 

Reply via email to