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 squeak414@free.fr:
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?
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)
...Stan
View this message in context: http://www.nabble.com/How-to-shorten-a-method-%28using-inject%3A-into%3A--%2... Sent from the Squeak - Beginners mailing list archive at Nabble.com.
Beginners mailing list Beginners@lists.squeakfoundation.org http://lists.squeakfoundation.org/mailman/listinfo/beginners
"cdrick" == cdrick cdrick65@gmail.com writes:
cdrick> Hi Stan, cdrick> 2008/7/21 stan shepherd squeak414@free.fr:
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?
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 <merlyn <at> stonehenge.com> writes:
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.
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
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:
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
"Zulq" == Zulq Alam me@zulq.net 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" == Randal L Schwartz merlyn@stonehenge.com 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.
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
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
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
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?
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
"cdrick" == cdrick cdrick65@gmail.com 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.
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
"K" == K K Subramaniam subbukk@gmail.com 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.
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?
"Zulq" == Zulq Alam me@zulq.net 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. :)
beginners@lists.squeakfoundation.org
-
cdrick -
K. K. Subramaniam -
merlyn＠stonehenge.com -
nicolas cellier -
stan shepherd -
Zulq Alam