Is this block used correctly?

[w***@gmail.com]

I created a Rational class. Inside this class, I used a function to reduce numerator and denominator so that the two are essentially prime to each other. I used a block to generate the gcd number. However, it seems that there is an error in the gcd block. Below is part of the code:

Rational extend [

reduce [
gcd := [ :x :y |
(x < y)
ifTrue: [ |temp| temp := x. "make sure x stores larger val"
x := y. "than y"
y := temp. ]
[(y=0) not] whileTrue: [ |r|
r := x \\ y.
x := y.
y := r. ]
^x. "when done, return the gcd"
]

"numer and denom" are defined as instanceVariableNames "
"for simplicity I assume numer and denom are not negative "

numer := numer // (gcd value: numer value: denom).
denom := denom // (gcd value: numer value: denom).

]

The error is in the line : [(y=0) not] whileTrue: [|r|

It says "parse error, expected '[' ". Does someone know what is wrong with the code?

Also, I would like to know if using block this way is correct.
Thank you very much

[s***@metanews.biz]

Didn't have time to really think about this, though I will later, but it looks on first glance as though

y := temp. ]

should probably be

y := temp].

Post by w***@gmail.com
Rational extend [
reduce [
gcd := [ :x :y |
(x < y)
ifTrue: [ |temp| temp := x. "make sure x stores larger val"
x := y. "than y"
y := temp. ]
[(y=0) not] whileTrue: [ |r|
r := x \\ y.
x := y.
y := r. ]
^x. "when done, return the gcd"
]
"numer and denom" are defined as instanceVariableNames "
"for simplicity I assume numer and denom are not negative "
numer := numer // (gcd value: numer value: denom).
denom := denom // (gcd value: numer value: denom).
]
The error is in the line : [(y=0) not] whileTrue: [|r|
It says "parse error, expected '[' ". Does someone know what is wrong with the code?
Also, I would like to know if using block this way is correct.
Thank you very much

[s***@metanews.biz]

I think my earlier answer is correct.

I'm not really exactly sure about the notation you are using to define this method.

Usually if you are creating something like a Rational class you would have two instance variables, denominator and numerator. reduce would be a method called to express the number with the smallest possible denominator.

There are a lot of discussions about balancing expression versus efficiency and so forth around Smalltalk, but I am a great believer in expression and code that is as compact as possible in terms of the number of lines used, so I would likely write something such as this in this way:

largeNum := (OrderedCollection with: x with: y) max.
smallNum := (OrderedCollection with: x with: y) min.
[smallNum ~= 0] whileTrue: [ |rmdr|
rmdr := largeNum \\ smallNum.
largeNum := smallNum.
smallNum := rmdr].
^ largeNum

You can save one more line of code with this:

((x / y) > 1) ifTrue: [ smallNum := x. largeNum := y ] ifFalse: [ smallNum := y. largeNum := x ].
[smallNum ~= 0] whileTrue: [ |rmdr|
rmdr := largeNum \\ smallNum.
largeNum := smallNum.
smallNum := rmdr].
^ largeNum

but it is just that little bit less clear about what is going on.

If you would like to write a bit more about what you are doing, I would be happy to reply.

rgds

Post by w***@gmail.com
Rational extend [
reduce [
gcd := [ :x :y |
(x < y)
ifTrue: [ |temp| temp := x. "make sure x stores larger val"
x := y. "than y"
y := temp. ]
[(y=0) not] whileTrue: [ |r|
r := x \\ y.
x := y.
y := r. ]
^x. "when done, return the gcd"
]
"numer and denom" are defined as instanceVariableNames "
"for simplicity I assume numer and denom are not negative "
numer := numer // (gcd value: numer value: denom).
denom := denom // (gcd value: numer value: denom).
]
The error is in the line : [(y=0) not] whileTrue: [|r|
It says "parse error, expected '[' ". Does someone know what is wrong with the code?
Also, I would like to know if using block this way is correct.
Thank you very much

[Richard Sargent]

Post by w***@gmail.com
Rational extend [
reduce [
gcd := [ :x :y |
(x < y)
ifTrue: [ |temp| temp := x. "make sure x stores larger val"
x := y. "than y"
y := temp. ]
[(y=0) not] whileTrue: [ |r|
r := x \\ y.
x := y.
y := r. ]
^x. "when done, return the gcd"
]
"numer and denom" are defined as instanceVariableNames "
"for simplicity I assume numer and denom are not negative "
numer := numer // (gcd value: numer value: denom).
denom := denom // (gcd value: numer value: denom).
]
The error is in the line : [(y=0) not] whileTrue: [|r|
It says "parse error, expected '[' ". Does someone know what is wrong with the code?
Also, I would like to know if using block this way is correct.
Thank you very much

As mentioned elsewhere, you need to end statements with a period (optional on the last statement of the method).

Possibly more important, most Smalltalks treat the arguments to a Block as read-only variables, so you should use extra temporary variables.

Also, always use names that are complete and informative, as much as possible. Easy heuristic: if you were to explain to a colleague what you had written and your explanation is not using the words you wrote, change the code to correspond to what you would have explained. e.g., if you would have to explain that "numer" is the numerator and "denom" is the denominator, they are misnamed.


Another writer suggested using "(OrderedCollection with: something with: anotherThing) max". This is wasteful when trying to determine the largest and smallest of two things, since the Number hierarchy usually includes the #max: and #min: selectors to work with two numbers. You want to avoid *unnecessary* object creation, unless doing so sacrifices too much clarity.