First commit to Pharoinbox. Need advice.

My last package is Balloon-hjo.72.mcz.
It is small testing & refactoring on Ballloon supporting collections.

I wonder this type of work is acceptable?

Best Regards

On 26 January 2011 08:13, HwaJong Oh <[hidden email]> wrote:
>
> My last package is Balloon-hjo.72.mcz.
> It is small testing & refactoring on Ballloon supporting collections.
>
> I wonder this type of work is acceptable?

Are you joking? Any contribution is acceptable. Of course if its pass
the quality test :)
And anything which improves functionality & cleans the code is acceptable.

Can i ask you to make life easier for review?
Please create entry in issue tracker , put a link to file there and a
little description of your changes.
This is to make life easier for people who will review your code and
integrate it. :)
Also, this will make integration process visible to all people.

>
> Best Regards
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237535.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

On 26.01.2011 08:13, HwaJong Oh wrote:
> My last package is Balloon-hjo.72.mcz.
> It is small testing&  refactoring on Ballloon supporting collections.
>
> I wonder this type of work is acceptable?
>
> Best Regards
Refactorings and more tests are certainly welcome!

Here's a few comments:
While I like keeping common functionality in one place, I'm not sure
creating a new helper object for each at:/at:put: access is a good idea.
Nor keeping the helper cached to avoid this. Have you considered using a
Trait instead? To me it seems like a good use-case for one.

  To the tests:
- setUp uses at:put:. testAt asserts the values set in setUp. Thus, is
it not really just another at:put: test? Does it test something not
covered by  the other at:put: test(s)?
- testAtPutLargeInteger uses a static integer. This is not ideal, as the
range of SmallInteger depends on platform (I realize though, that it is
large enough for it not to fail on 64bit as well).
Using f.ex SmallInteger maxVal + staticInteger is more intention revealing.
Although, max/minVal actually returns a constant at the moment rather
than a platform-specific value, and thus would fail on 64bit. I'd
consider this a bug which it'd be nice if a test exposed :)
- testWriteXYEndianOn uses 256*256*256. etc. Probably down to
preference, but I think they'd looks nicer using the bitShift: (or #<<
if you prefer less parenthesis) operator.

Cheers,
Henry

Thank you for your detailed advice.
Although i could only comply bitShift: adivce. Using trait was unsuccessful. super send is not possible with traits and applying PointArrayTrait on ShortPointArray makes strange error.

New version is out. (Balloon-hjo.73)

At least my tests are passing, so minimum quality is guarrentied. Wonder what the quality tester want. :-)

Is this where i should go?  http://code.google.com/p/pharo/issues/list

On 26 January 2011 12:47, HwaJong Oh <[hidden email]> wrote:
>
> At least my tests are passing, so minimum quality is guarrentied. Wonder what
> the quality tester want. :-)
>
Anything which you would actually want to see being on his place :)

> Is this where i should go?  http://code.google.com/p/pharo/issues/list

Yes.

> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237883.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

On 26.01.2011 12:40, HwaJong Oh wrote:
> Thank you for your detailed advice.
> Although i could only comply bitShift: adivce. Using trait was unsuccessful.
> super send is not possible with traits and applying PointArrayTrait on
> ShortPointArray makes strange error.
>
> New version is out. (Balloon-hjo.73)
Strange, using a trait worked fine when I tried it, all tests passing,
see v74 in inbox for details.

Cheers,
Henry

On Jan 26, 2011, at 12:40 PM, HwaJong Oh wrote:

>
> Thank you for your detailed advice.
> Although i could only comply bitShift: adivce. Using trait was unsuccessful.
> super send is not possible with traits and applying PointArrayTrait on
> ShortPointArray makes strange error.

super send are possible with traits.
Tell us what was your problem.

Stef

>
> New version is out. (Balloon-hjo.73)
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237870.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>

+1

Yes, you are right. I was wrong. There is nothing super send. I had error editing my trait codes around "super", so i thought trait can super send.  
 http://screencast.com/t/yyjLLm0S

MoveMethod:

FriendOfPointArray >>
at: index

        ^(pointArray superAt: index * 2 - 1) @ (pointArray superAt: index * 2)

to

PointArrayTrait >>
at: index

        ^(super at: index * 2 - 1) @ (super at: index * 2)


while editing "pointArray superAt:" to "super at:", the editor made error.

Copying the code to native Mac editor, edit them, then copy back to Pharo editor suceeded.

On 27 January 2011 07:28, HwaJong Oh <[hidden email]> wrote:
Hehe.. funny mistake :)

Btw, what is this FriendOfPointArray? The name sounds strange

> Copying the code to native Mac editor, edit them, then copy back to Pharo
> editor suceeded.
>
>
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3241521.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

On 27.01.2011 17:38, Igor Stasenko wrote: Yeah, I created an issue for it:
http://code.google.com/p/pharo/issues/detail?id=3603
> Btw, what is this FriendOfPointArray? The name sounds strange
>
It actually has a class comment :)
"Removes duplication of PointArray & ShortPointArray. They do not share
same superclass, therefore this class is to remove to duplicated codes
by extracting class"

Hence my suggestion of a trait to keep it in rather than creating a new
helper instance for each at: or at:put:

Cheers,
Henry