A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-ct.410.mcz
==================== Summary ====================
Name: Graphics-ct.410 Author: ct Time: 14 August 2019, 11:52:24.4754 pm UUID: 008f8bb1-9963-1e46-8b29-1f1e2aa3235b Ancestors: Graphics-nice.409
Little refactoring to HSV conversion
Afaik #caseOf: is compiler optimized, so this is not only more beautiful but also at least at efficient as the old conditional chain, isn't it?
=============== Diff against Graphics-nice.409 ===============
Item was changed: ----- Method: Color>>setHue:saturation:brightness: (in category 'private') ----- setHue: hue saturation: saturation brightness: brightness "Initialize this color to the given hue, saturation, and brightness. See the comment in the instance creation method for details."
| s v hf i f p q t | s := (saturation asFloat max: 0.0) min: 1.0. v := (brightness asFloat max: 0.0) min: 1.0.
"zero saturation yields gray with the given brightness" s = 0.0 ifTrue: [ ^ self setRed: v green: v blue: v ].
hf := hue asFloat. (hf < 0.0 or: [hf >= 360.0]) ifTrue: [hf := hf \ 360]. hf := hf / 60.0. i := hf asInteger. "integer part of hue" f := hf fractionPart. "fractional part of hue" p := (1.0 - s) * v. q := (1.0 - (s * f)) * v. t := (1.0 - (s * (1.0 - f))) * v.
+ ^ i caseOf: { + [0] -> [ self setRed: v green: t blue: p ]. + [1] -> [ self setRed: q green: v blue: p ]. + [2] -> [ self setRed: p green: v blue: t ]. + [3] -> [ self setRed: p green: q blue: v ]. + [4] -> [ self setRed: t green: p blue: v ]. + [5] -> [ self setRed: v green: p blue: q ]. }! - 0 = i ifTrue: [ ^ self setRed: v green: t blue: p ]. - 1 = i ifTrue: [ ^ self setRed: q green: v blue: p ]. - 2 = i ifTrue: [ ^ self setRed: p green: v blue: t ]. - 3 = i ifTrue: [ ^ self setRed: p green: q blue: v ]. - 4 = i ifTrue: [ ^ self setRed: t green: p blue: v ]. - 5 = i ifTrue: [ ^ self setRed: v green: p blue: q ]. - - self error: 'implementation error'. - !
[Color basicNew setHue: (-360 to: 720 by: 0.1) atRandom saturation: (-1 to: 2 by: 0.1) atRandom brightness: (-1 to: 2 by: 0.1) atRandom] benchFor: 180 seconds. "-> '300,000 per second. 3.33 microseconds per run.'" [Color basicNew setHue: (-360 to: 720 by: 0.1) atRandom saturation: (-1 to: 2 by: 0.1) atRandom brightness: (-1 to: 2 by: 0.1) atRandom] benchFor: 180 seconds. "-> '370,000 per second. 2.71 microseconds per run.'"
Seems to be even faster. :o
-- Sent from: http://forum.world.st/Squeak-Dev-f45488.html
On 14.08.2019, at 23:52, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-ct.410.mcz
==================== Summary ====================
Name: Graphics-ct.410 Author: ct Time: 14 August 2019, 11:52:24.4754 pm UUID: 008f8bb1-9963-1e46-8b29-1f1e2aa3235b Ancestors: Graphics-nice.409
Little refactoring to HSV conversion
Afaik #caseOf: is compiler optimized, so this is not only more beautiful but also at least at efficient as the old conditional chain, isn't it?
-1.
I do not hold that caseOf is more beautiful. It may be a matter of taste, here. But often it's an instance of forgotten polymorphy, so I'm against promoting it outside "important" uses, eg in Parser or so.
I see that this is a lot number-dancing and maybe not even an important method, so it might not matter much. Yet I don't see benefit in the #caseOf variant.
Best regards -Tobias
=============== Diff against Graphics-nice.409 ===============
Item was changed: ----- Method: Color>>setHue:saturation:brightness: (in category 'private') ----- setHue: hue saturation: saturation brightness: brightness "Initialize this color to the given hue, saturation, and brightness. See the comment in the instance creation method for details."
| s v hf i f p q t | s := (saturation asFloat max: 0.0) min: 1.0. v := (brightness asFloat max: 0.0) min: 1.0.
"zero saturation yields gray with the given brightness" s = 0.0 ifTrue: [ ^ self setRed: v green: v blue: v ].
hf := hue asFloat. (hf < 0.0 or: [hf >= 360.0]) ifTrue: [hf := hf \ 360]. hf := hf / 60.0. i := hf asInteger. "integer part of hue" f := hf fractionPart. "fractional part of hue" p := (1.0 - s) * v. q := (1.0 - (s * f)) * v. t := (1.0 - (s * (1.0 - f))) * v.
- ^ i caseOf: {
[0] -> [ self setRed: v green: t blue: p ].
[1] -> [ self setRed: q green: v blue: p ].
[2] -> [ self setRed: p green: v blue: t ].
[3] -> [ self setRed: p green: q blue: v ].
[4] -> [ self setRed: t green: p blue: v ].
[5] -> [ self setRed: v green: p blue: q ]. }!
- 0 = i ifTrue: [ ^ self setRed: v green: t blue: p ].
- 1 = i ifTrue: [ ^ self setRed: q green: v blue: p ].
- 2 = i ifTrue: [ ^ self setRed: p green: v blue: t ].
- 3 = i ifTrue: [ ^ self setRed: p green: q blue: v ].
- 4 = i ifTrue: [ ^ self setRed: t green: p blue: v ].
- 5 = i ifTrue: [ ^ self setRed: v green: p blue: q ].
- self error: 'implementation error'.
- !
Hmm... looking at PNMReadWriter >> #nextImage or MimeConverter class >> #forEncoding:, the use in Color looks similar and valid. What about the otherwise-case and the former "implementation error"? A case error would have a different meaning?
However, Tobias is right that there are only 15+74 uses of #caseOf:(otherwise:) in the image. We should review those carefully.
Best, Marcel Am 15.08.2019 02:16:53 schrieb Tobias Pape das.linux@gmx.de:
On 14.08.2019, at 23:52, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-ct.410.mcz
==================== Summary ====================
Name: Graphics-ct.410 Author: ct Time: 14 August 2019, 11:52:24.4754 pm UUID: 008f8bb1-9963-1e46-8b29-1f1e2aa3235b Ancestors: Graphics-nice.409
Little refactoring to HSV conversion
Afaik #caseOf: is compiler optimized, so this is not only more beautiful but also at least at efficient as the old conditional chain, isn't it?
-1.
I do not hold that caseOf is more beautiful. It may be a matter of taste, here. But often it's an instance of forgotten polymorphy, so I'm against promoting it outside "important" uses, eg in Parser or so.
I see that this is a lot number-dancing and maybe not even an important method, so it might not matter much. Yet I don't see benefit in the #caseOf variant.
Best regards -Tobias
=============== Diff against Graphics-nice.409 ===============
Item was changed: ----- Method: Color>>setHue:saturation:brightness: (in category 'private') ----- setHue: hue saturation: saturation brightness: brightness "Initialize this color to the given hue, saturation, and brightness. See the comment in the instance creation method for details."
| s v hf i f p q t | s := (saturation asFloat max: 0.0) min: 1.0. v := (brightness asFloat max: 0.0) min: 1.0.
"zero saturation yields gray with the given brightness" s = 0.0 ifTrue: [ ^ self setRed: v green: v blue: v ].
hf := hue asFloat. (hf = 360.0]) ifTrue: [hf := hf \ 360]. hf := hf / 60.0. i := hf asInteger. "integer part of hue" f := hf fractionPart. "fractional part of hue" p := (1.0 - s) * v. q := (1.0 - (s * f)) * v. t := (1.0 - (s * (1.0 - f))) * v.
- ^ i caseOf: {
- [0] -> [ self setRed: v green: t blue: p ].
- [1] -> [ self setRed: q green: v blue: p ].
- [2] -> [ self setRed: p green: v blue: t ].
- [3] -> [ self setRed: p green: q blue: v ].
- [4] -> [ self setRed: t green: p blue: v ].
- [5] -> [ self setRed: v green: p blue: q ]. }!
- 0 = i ifTrue: [ ^ self setRed: v green: t blue: p ].
- 1 = i ifTrue: [ ^ self setRed: q green: v blue: p ].
- 2 = i ifTrue: [ ^ self setRed: p green: v blue: t ].
- 3 = i ifTrue: [ ^ self setRed: p green: q blue: v ].
- 4 = i ifTrue: [ ^ self setRed: t green: p blue: v ].
- 5 = i ifTrue: [ ^ self setRed: v green: p blue: q ].
- self error: 'implementation error'.
- !
What should be the general problem with #caseOf:? Just in this example, I think #caseOf: provides a better alternative to an amount of logic. #caseOf: describes a more data-driven approach to express repeating logic. In particular when you don't have a default case, you won't need to explicitly write a check for that, but #caseOf: will handle that for you (so I think in the current example, a caseError just has the right semantic. Apart from that I think errors that are never ever expected to be raised are some kind of code smell).
Of course, excessive use of #caseOf: might indicate the lack of an inheritance tree, but I don't think this is specific to #caseOf:, the same smell can be written with many conditionals as well.
Best,
Christoph
________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Donnerstag, 15. August 2019 10:26:00 An: gettimothy via Squeak-dev Betreff: Re: [squeak-dev] The Inbox: Graphics-ct.410.mcz
Hmm... looking at PNMReadWriter >> #nextImage or MimeConverter class >> #forEncoding:, the use in Color looks similar and valid. What about the otherwise-case and the former "implementation error"? A case error would have a different meaning?
However, Tobias is right that there are only 15+74 uses of #caseOf:(otherwise:) in the image. We should review those carefully.
Best, Marcel
Am 15.08.2019 02:16:53 schrieb Tobias Pape das.linux@gmx.de:
On 14.08.2019, at 23:52, commits@source.squeak.org wrote:
A new version of Graphics was added to project The Inbox: http://source.squeak.org/inbox/Graphics-ct.410.mcz
==================== Summary ====================
Name: Graphics-ct.410 Author: ct Time: 14 August 2019, 11:52:24.4754 pm UUID: 008f8bb1-9963-1e46-8b29-1f1e2aa3235b Ancestors: Graphics-nice.409
Little refactoring to HSV conversion
Afaik #caseOf: is compiler optimized, so this is not only more beautiful but also at least at efficient as the old conditional chain, isn't it?
-1.
I do not hold that caseOf is more beautiful. It may be a matter of taste, here. But often it's an instance of forgotten polymorphy, so I'm against promoting it outside "important" uses, eg in Parser or so.
I see that this is a lot number-dancing and maybe not even an important method, so it might not matter much. Yet I don't see benefit in the #caseOf variant.
Best regards -Tobias
=============== Diff against Graphics-nice.409 ===============
Item was changed: ----- Method: Color>>setHue:saturation:brightness: (in category 'private') ----- setHue: hue saturation: saturation brightness: brightness "Initialize this color to the given hue, saturation, and brightness. See the comment in the instance creation method for details."
| s v hf i f p q t | s := (saturation asFloat max: 0.0) min: 1.0. v := (brightness asFloat max: 0.0) min: 1.0.
"zero saturation yields gray with the given brightness" s = 0.0 ifTrue: [ ^ self setRed: v green: v blue: v ].
hf := hue asFloat. (hf < 0.0="" or:="" [hf="">= 360.0]) ifTrue: [hf := hf \ 360]. hf := hf / 60.0. i := hf asInteger. "integer part of hue" f := hf fractionPart. "fractional part of hue" p := (1.0 - s) * v. q := (1.0 - (s * f)) * v. t := (1.0 - (s * (1.0 - f))) * v.
- ^ i caseOf: {
- [0] -> [ self setRed: v green: t blue: p ].
- [1] -> [ self setRed: q green: v blue: p ].
- [2] -> [ self setRed: p green: v blue: t ].
- [3] -> [ self setRed: p green: q blue: v ].
- [4] -> [ self setRed: t green: p blue: v ].
- [5] -> [ self setRed: v green: p blue: q ]. }!
- 0 = i ifTrue: [ ^ self setRed: v green: t blue: p ].
- 1 = i ifTrue: [ ^ self setRed: q green: v blue: p ].
- 2 = i ifTrue: [ ^ self setRed: p green: v blue: t ].
- 3 = i ifTrue: [ ^ self setRed: p green: q blue: v ].
- 4 = i ifTrue: [ ^ self setRed: t green: p blue: v ].
- 5 = i ifTrue: [ ^ self setRed: v green: p blue: q ].
- self error: 'implementation error'.
- !
On 2019-08-15, at 2:10 AM, Thiede, Christoph Christoph.Thiede@student.hpi.uni-potsdam.de wrote:
Of course, excessive use of #caseOf: might indicate the lack of an inheritance tree, but I don't think this is specific to #caseOf:, the same smell can be written with many conditionals as well.
It is a long understood truism that there is no language in which the poor programmer cannot write FORTRAN.
I've long ago lost count of the number of times I have seen stuff like
(thing isKindOf: Weeble) ifTrue:["do something daft"]. (thing isKindOf: Blobby) ifFalse: ["do related ridiculous thing that could be factored properly"]. thing doodah caseOf: .... etc etc
I used to find it disheartening. Now I just find it tedious and occasionally profitable as I get paid to make it right.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Oxymorons: Government organization
I moved this submission to the treated inbox because we seem to agree that caseOf does not improve readability here.
In general, I would try to preserve the original meaning, which would have been that "self error: 'implementation error'." be the otherwise-clause.
Best, Marcel Am 15.08.2019 18:59:06 schrieb tim Rowledge tim@rowledge.org:
On 2019-08-15, at 2:10 AM, Thiede, Christoph wrote:
Of course, excessive use of #caseOf: might indicate the lack of an inheritance tree, but I don't think this is specific to #caseOf:, the same smell can be written with many conditionals as well.
It is a long understood truism that there is no language in which the poor programmer cannot write FORTRAN.
I've long ago lost count of the number of times I have seen stuff like
(thing isKindOf: Weeble) ifTrue:["do something daft"]. (thing isKindOf: Blobby) ifFalse: ["do related ridiculous thing that could be factored properly"]. thing doodah caseOf: .... etc etc
I used to find it disheartening. Now I just find it tedious and occasionally profitable as I get paid to make it right.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Oxymorons: Government organization
squeak-dev@lists.squeakfoundation.org