How to shorten a method (using inject: into: ?)

Hi, I have the following method, that code critics flags as long:

testMaleMeiosis2
        | testSet mCount pCount mTotal pTotal |
        pTotal := 0.
        mTotal := 0.
        Forecaster testMale meiose
                do: [:strand |
                        testSet := strand testRun.
                        mCount := testSet maternalCount.
                        pCount := testSet paternalCount.
                        mTotal := mTotal + mCount.
                        pTotal := pTotal + pCount].
        self
                should: [mTotal = 100
                                and: [pTotal = 100]]

I'm trying to shorten it, including various attempts with inject: into: , but with no success. Any tips please?

...Stan

Hi Stan,

2008/7/21 stan shepherd <[hidden email]>: First I would do that:

testMaleMeiosis2
       | testSet  mTotal pTotal |
       pTotal := 0.
       mTotal := 0.
       Forecaster testMale meiose
               do: [:strand |
                       testSet := strand testRun.
                       mTotal := mTotal + testSet maternalCount.
                       pTotal := pTotal + testSet paternalCount].
       self
               should: [mTotal = 100
                               and: [pTotal = 100]].

then maybe:

testMaleMeiosis2
| testSet maternalTotal paternalTotal|
testSet := strand testRun.
maternalTotal := Forecaster testMale meiose
        inject: 0
        into: [:mTotal :val | mTotal + testSet maternalCount].
paternalTotal := Forecaster testMale meiose
        inject: 0
        into: [:mTotal :val | pTotal + testSet paternalCount].

self should: [mTotal = 100 and: [pTotal = 100]].

"you might call sub-methods for maternalCount and parentalCount"

or to do in one method but this is not clear... you con inject anArray
containing you results like:

 #(1 2 3 )
        inject: #(1 1)
        into: [:array :elem |
                        array at: 1 put: (array at: 1) * elem.
                        array at: 2 put: (array at: 2) + elem.
                        array]

--> returns #(6 7)

So somethink like that should work:

testMaleMeiosis2
| resultArray tesSet |
testSet := strand testRun.
resultArray := Forecaster testMale meiose
        inject: #(0 0)
        into: [:array :val |
             array at: 1 put: (array at: 1) + testSet maternalCount.
             array at: 2 put: (array at: 2) + testSet paternalCount.
             array]

self should: [ resultArray = #(100 100)].


I nevertheless prefer to have two seprate inject method as I find more
readable. When methods are too long, you can split in several methods
too. I may also put testSet as an instvar if used in other places...

testMaleMeiosis2
| maternalTotal paternalTotal|
maternalTotal := self computeMaternalTotal.
paternalTotal := selfComputePaternalTotal.
self should: [maternalTotal = 100 and: [paternalTotal =100]].

HTH

Cédrick


The problem is you have two operations in the block but I tried that
and it seems to work (passing an array with your mTotal and pTotal)



_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "cdrick" == cdrick  <[hidden email]> writes:

cdrick> Hi Stan,
cdrick> 2008/7/21 stan shepherd <[hidden email]>:
cdrick> So somethink like that should work:

cdrick> testMaleMeiosis2
cdrick> | resultArray tesSet |
cdrick> testSet := strand testRun.
cdrick> resultArray := Forecaster testMale meiose
cdrick> inject: #(0 0)
cdrick>         into: [:array :val |
cdrick>              array at: 1 put: (array at: 1) + testSet maternalCount.
cdrick>     array at: 2 put: (array at: 2) + testSet paternalCount.
cdrick>     array]

cdrick> self should: [ resultArray = #(100 100)].

This looks even uglier.  How about first gathering the testSets, then
getting what you need from those:

| testSets mTotal pTotal |

testSets := (Forecaster testMale meiose) collect: [:strand | strand testRun ].
mTotal := (testSets collect: [:each | each maternalCount]) sum.
pTotal := (testSets collect: [:each | each paternalCount]) sum.

Unless you're talking about 5000 elements in testSets, this is likely in the
same ballpark for speed, and that's probably dwarfed by the cost of #testRun
anyway.

Sometimes an obsession with #inject:into: as a shiny object leads you down a
dark path.  Especially with sums.  Consider #sum first.

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

Hi Stan,

How about using #detectSum:?

testMaleMeiosis2
        pTotal := Forecaster testMale meiose
                detectSum: [:strand | strand testRun paternalCount].
        mTotal := Forecaster testMale meiose
                detectSum: [:strand | strand testRun maternalCount].
        self should: [mTotal = 100 and: [pTotal = 100]]

Other methods of this variety include #detectMin:, #detectMax: and
#count: and possibly others.

Regards,
Zulq.

stan shepherd wrote:
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "Zulq" == Zulq Alam <[hidden email]> writes:

Zulq> Hi Stan,
Zulq> How about using #detectSum:?

Zulq> testMaleMeiosis2
Zulq> pTotal := Forecaster testMale meiose
Zulq> detectSum: [:strand | strand testRun paternalCount].
Zulq> mTotal := Forecaster testMale meiose
Zulq> detectSum: [:strand | strand testRun maternalCount].
Zulq> self should: [mTotal = 100 and: [pTotal = 100]]

Huh?  Why the heck is that called #detectSum: instead of just #sum:?
I was trying to find that.  That'd shorten my previous one even more:

testRuns := Forecaster testMale meiose collect: [:e | e testRun].
pTotal := testRuns detectSum: [:e | e paternalCount].
mTotal := testRuns detectSum: [:e | e maternalCount].

No redundant calculations, although we're building an array in the middle that
will be a throwaway.

/me makes a note to look under #detectFoo: next time

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "Randal" == Randal L Schwartz <[hidden email]> writes:

Randal> Huh?  Why the heck is that called #detectSum: instead of just #sum:?

[...]

Randal> /me makes a note to look under #detectFoo: next time

In fact, now that I look at it, it's really not like #detectMax:, which
returns the *original* element that is the max, or #detect:, which returns
an *original* element that meets a test.  It's transformational, more
like #collect.  But since #sum already does that, it really should have
simply been named #sum:.

Sigh.

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

Quoting cdrick <cdrick65@gmail.com>:
>
> testMaleMeiosis2
> | resultArray tesSet |
> testSet := strand testRun.
> resultArray := Forecaster testMale meiose
> inject: #(0 0)
>         into: [:array :val |
>              array at: 1 put: (array at: 1) + testSet maternalCount.
>     array at: 2 put: (array at: 2) + testSet paternalCount.
>     array]
>
>> self should: [ resultArray = #(100 100)].
>

thanks Cédrick. Yes, that's what I was trying to do earlier, but without success. Perhaps a missing explicit return somewhere.

Quoting "Randal L. Schwartz" <merlyn@stonehenge.com>:
>
> | testSets mTotal pTotal |
>
> testSets := (Forecaster testMale meiose) collect: [:strand | strand testRun
> ].
> mTotal := (testSets collect: [:each | each maternalCount]) sum.
> pTotal := (testSets collect: [:each | each paternalCount]) sum.
>

Randal, much more succinct than my effort, thanks.

Quoting Zulq Alam <me@zulq.net>:

> Hi Stan,
>
> How about using #detectSum:?
>
>testMaleMeiosis2                
>        pTotal := Forecaster testMale meiose
>                detectSum: [:strand | strand testRun paternalCount].
>        mTotal := Forecaster testMale meiose
>                detectSum: [:strand | strand testRun maternalCount].        
>       self should: [mTotal = 100 and: [pTotal = 100]]  
>
>

detectSum:  neatens things up nicely, Zulq. I dont think I can call meiose twice in this case, because it returns different strands each time, intentionally. It randomly swaps genes between strands as in genetic recombination, and that's what I'm trying to test.

So until the next cunning suggestion, at the moment it looks like:

testMaleMeiosis3
        | testSets mTotal pTotal |
        testSets := Forecaster testMale meiose
                                collect: [:strand | strand testRun].
        mTotal := testSets
                                detectSum: [:each | each maternalCount].
        pTotal := testSets
                                detectSum: [:each | each paternalCount].
        self
                should: [mTotal = 100
                                and: [pTotal = 100]]

I appreciate all your replies; very illuminating.

...Stan

Randal L. Schwartz <merlyn <at> stonehenge.com> writes:

Premature optimization might make our code obscure for no serious reasons.
That means hard to read, hard to maintain, hard to test, maybe flawed.

However, if you deal with really huge collections, problems will come more from
space reasons.

nLoop := 500000.
example1 := [(1 to: nLoop) detectSum: [:each | each log: 10]].
example2 := [((1 to: nLoop) collect: [:each | each log: 10]) sum].

the main difference between 1 and 2 won't come from iterating twice
(as this is partially masked by the cost of #log: evaluation).
It will come mainly from putting pressure on garbage collector:
- second example allocates a lot of new Float objects,
- but unlike first example, these objects cannot be reclaimed immediately.

{
[Smalltalk garbageCollect. example1 timeToRun] value.
[Smalltalk garbageCollect. example2 timeToRun] value.
}
exhibits a factor 12 on my 3.10 image, not a factor 2.

If you plot it, example2 timeToRun is not proportional to nLoop.
It can be worse once you trigger OS page swapping on disk.

I remember old st-80 implementations would eventually have crashed on this kind
of example, because the lowSpace process could be activated too late (every 50ms
or so). Fortunately, I think this is not the case any more.

Of course, up to 10000, you will hardly notice a difference between 1 and 2.
Better leave the code simple.
Here detectSum: is both simple and efficient, though i do not like the name.
(I usually name the message sumOf:).

Nicolas

_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

Using #inject:into and Point objects to store the totals:

testMaleMeiosis4
   | totals |
   totals := Forecaster testMale meiose inject: 0@0 into:
     [:subtotals :strand |
     subtotals + (strand maternalCount @ strand maternalCount)].
   self should: [totals = 100@100]

The point notation makes it look very clean (to me).

Z

stan shepherd wrote:

> So until the next cunning suggestion

_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

Zulq Alam-2 wrote

Using #inject:into and Point objects to store the totals:

testMaleMeiosis4
   | totals |
   totals := Forecaster testMale meiose inject: 0@0 into:
     [:subtotals :strand |
     subtotals + (strand maternalCount @ strand maternalCount)].
   self should: [totals = 100@100]

The point notation makes it look very clean (to me).

I agree it's very tidy looking.

Is there a style issue with introducing an object type which is not part of the problem? Is point often used like this- it's being treated as a shorthand form of an array of 2 elements which has some handy methods, is it not?

...Stan

to me same problem as earlier with the array even if a bit more
readable. Actually it's more elgant and shorter thant the array
solution but the intention is even more hidden (to me ;) ).
That's cool we can do that but not to use except for optimization (I'm
not sure though this will be quicker).

Always choose the most readable (before optimizing) and to me, the
following is much more readable:

mTotal := (testSets collect: [:each | each maternalCount]) sum.
pTotal := (testSets collect: [:each | each paternalCount]) sum.

my 2 cents

Cédrick

_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "cdrick" == cdrick  <[hidden email]> writes:

cdrick> Always choose the most readable (before optimizing) and to me, the
cdrick> following is much more readable:

cdrick> mTotal := (testSets collect: [:each | each maternalCount]) sum.
cdrick> pTotal := (testSets collect: [:each | each paternalCount]) sum.

Well, the #detectSum: solution is even cleaner. :)

  mTotal := testSets detectSum: [:each | each maternalCount].

It's just a horribly named method, which is why I didn't find it at first.

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

On Tuesday 22 Jul 2008 9:54:25 pm Randal L. Schwartz wrote:
> It's just a horribly named method, which is why I didn't find it at first.
Message Finder is quite handy for exploring selectors in an image.
Entering "sum" in a new line in workspace or text object and then pressing
ALT+SHIFT+W would have revealed detectSum: (it is eleventh in my list).

Subbu
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

Yes, it's not a good idea to couple your classes with others for
frivolous reasons. I just wanted to see how tersely it could be
expressed with #inject:into:.

Z.

stan shepherd wrote:

>
> Is there a style issue with introducing an object type which is not part of
> the problem? Is point often used like this- it's being treated as a
> shorthand form of an array of 2 elements which has some handy methods, is it
> not?
>


_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "K" == K K Subramaniam <[hidden email]> writes:

K> Entering "sum" in a new line in workspace or text object and then pressing
K> ALT+SHIFT+W would have revealed detectSum: (it is eleventh in my list).

Yeah, I looked near the implementation of #sum instead.  Ooops.

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners

>>>>> "Zulq" == Zulq Alam <[hidden email]> writes:

Zulq> Yes, it's not a good idea to couple your classes with others for frivolous
Zulq> reasons. I just wanted to see how tersely it could be expressed with
Zulq> #inject:into:.

And now I hope you see why I say that #inject:into: is both overutilized
and underutilized. :)

--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[hidden email]> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion
_______________________________________________
Beginners mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/beginners