how to rewrite this to use a stream
It's bad advice in this case, but correctly indicates there is a problem with your code. See below. On Thu, Dec 13, 2018 at 8:13 AM Roelof Wobben <r.wobben@home.nl> wrote:
Hello,
I have this code :
FindAndDeleteWordsWithForbiddenParts "deletes all the words that contain forbidden parts"
| result | result := words select: [ :word | [ (forbiddenWords reject: [ :forbidden | '*' , forbidden , '*' match: word ]) isNotEmpty ]
Constructing a pattern and pattern matching is a heavy-handed approach to find out if a word contains a substring. I'm not familiar with Pharo's methods, but at the least String inherits #indexOfSubCollection:startingAt: and probably a number of others. There is also a good chance String inherits or implements a more specific method that will test for the presence of a sub-string.
A good piece of advice I once received is (paraphrased as): Spend more time reading and less time writing. :-) The deeper a class hierarchy, the greater the chance that the exact thing you need has already been implemented in the leaf class or its abstraction.
on: NotFound do: [ false ] ]. ^ result
but I see warnings that I have to use a stream instead of string concentation.
Anyone hints how to do so ?
Roelof
On Thu, Dec 13, 2018 at 12:44 PM Roelof Wobben <r.wobben@home.nl> wrote:
there is a method but then I ran into another warning
FindAndDeleteWordsWithForbiddenParts "deletes all the words that contain forbidden parts"
| result | result := words select: [ :word | [ (forbiddenWords detect: [ :forbidden | word includesAll: forbidden ]) = true ] on: NotFound do: [ true ] ].
There are a number of things you can do, such as the variations on detect , for example. But, even those keep you from the answer. The question you are trying to answer is which words satisfy the condition that they don't include any forbidden word as a sub-string. There are other variations than detect for that kind of question. Try examining the API in Collection for any which suggest a correspondence, and if not there, check subclasses in turn down to the classes of the collections you are working with. And don't forget that #select: isn't the only way to extract a subset of a collection. Look around for others. [I'm deliberately not giving you the answer, because I think you will benefit from learning how to work effectively in the environment.] ^ result
then I see a unnecearry = true but if I deleted that part the code does not what I was intented to do.
Roelof
Op 13-12-2018 om 18:15 schreef Richard Sargent:
It's bad advice in this case, but correctly indicates there is a problem with your code. See below.
On Thu, Dec 13, 2018 at 8:13 AM Roelof Wobben <r.wobben@home.nl> wrote:
Hello,
I have this code :
FindAndDeleteWordsWithForbiddenParts "deletes all the words that contain forbidden parts"
| result | result := words select: [ :word | [ (forbiddenWords reject: [ :forbidden | '*' , forbidden , '*' match: word ]) isNotEmpty ]
Constructing a pattern and pattern matching is a heavy-handed approach to find out if a word contains a substring. I'm not familiar with Pharo's methods, but at the least String inherits #indexOfSubCollection:startingAt: and probably a number of others. There is also a good chance String inherits or implements a more specific method that will test for the presence of a sub-string.
A good piece of advice I once received is (paraphrased as): Spend more time reading and less time writing. :-) The deeper a class hierarchy, the greater the chance that the exact thing you need has already been implemented in the leaf class or its abstraction.
on: NotFound do: [ false ] ]. ^ result
but I see warnings that I have to use a stream instead of string concentation.
Anyone hints how to do so ?
Roelof
Starting from the top:\ (0) It's not *wrong* to start a selector with a capital letter, but it is certainly unusual. (1) Intention-revealing names are good. That's why this name is bad. It does not *delete* anything. (2) The comment again says "delete" but nothing is deleted. Comments that mislead are worse than no comments. (3) An issue I have seen over and over in student code (in other programming languages) is writing regular expressions as string literals where they are used, so that every time you try a match the regular expression gets recompiled, over and over again. You're not exactly doing that, but you *are* rebuilding patterns over and over again. It would make more sense to hold a forbiddenPatterns collection. (4) You are saying "(coll reject: [..test..]) notEmpty", building an entire collection just to ask whether it has any members. What you want to know is "are there any elements of coll that do not satisfy the test", which can be expressed as (coll allSatisfy: [..test..]) not. (Sadly there is no #notAllSatisfy:, although #notAnySatisfy: *does* exist under the name #noneSatisfy:. (5) #match: is a highly risky way to look for a substring. What if you want to look for a literal # or * ? What if you need to look for 'Boring' but not 'boring'? ("Boring" is a perfectly good surname.) (6) You really don't need the "result" variable. allowedWords "Answer those words which do not contain any forbiddenWord as a substring." ^words select: [:each | forbiddenWords noneSatisfy: [:forbidden | <<each includes forbidden as a substring>>]] (7) You can find out how to do the <<part>> by looking at the methods for String. You will find several candidate methods. (8) Of course, if there are W words and F forbidden words, in the worst case you'll do W*F substring inclusion tests. This is not good. "Multiple pattern matching" is a much-studied problem, see for example Wu and Manber's paper http://web.archive.org/web/20130203031412/http://www.inf.fu-berlin.de/inst/a... There is more recent work also. Does anyone know whether there's a multiple pattern algorithm for Pharo? One could always fall back on ternary search trees... On Fri, 14 Dec 2018 at 05:13, Roelof Wobben <r.wobben@home.nl> wrote:
Hello,
I have this code :
FindAndDeleteWordsWithForbiddenParts "deletes all the words that contain forbidden parts"
| result | result := words select: [ :word | [ (forbiddenWords reject: [ :forbidden | '*' , forbidden , '*' match: word ]) isNotEmpty ] on: NotFound do: [ false ] ]. ^ result
but I see warnings that I have to use a stream instead of string concentation.
Anyone hints how to do so ?
Roelof
Thx for those reviews. And interesting paper. Phil On Sat, Dec 15, 2018, 04:20 Richard O'Keefe <raoknz@gmail.com wrote:
Starting from the top:\ (0) It's not *wrong* to start a selector with a capital letter, but it is certainly unusual. (1) Intention-revealing names are good. That's why this name is bad. It does not *delete* anything. (2) The comment again says "delete" but nothing is deleted. Comments that mislead are worse than no comments. (3) An issue I have seen over and over in student code (in other programming languages) is writing regular expressions as string literals where they are used, so that every time you try a match the regular expression gets recompiled, over and over again. You're not exactly doing that, but you *are* rebuilding patterns over and over again. It would make more sense to hold a forbiddenPatterns collection. (4) You are saying "(coll reject: [..test..]) notEmpty", building an entire collection just to ask whether it has any members. What you want to know is "are there any elements of coll that do not satisfy the test", which can be expressed as (coll allSatisfy: [..test..]) not. (Sadly there is no #notAllSatisfy:, although #notAnySatisfy: *does* exist under the name #noneSatisfy:. (5) #match: is a highly risky way to look for a substring. What if you want to look for a literal # or * ? What if you need to look for 'Boring' but not 'boring'? ("Boring" is a perfectly good surname.) (6) You really don't need the "result" variable.
allowedWords "Answer those words which do not contain any forbiddenWord as a substring." ^words select: [:each | forbiddenWords noneSatisfy: [:forbidden | <<each includes forbidden as a substring>>]]
(7) You can find out how to do the <<part>> by looking at the methods for String. You will find several candidate methods. (8) Of course, if there are W words and F forbidden words, in the worst case you'll do W*F substring inclusion tests. This is not good. "Multiple pattern matching" is a much-studied problem, see for example Wu and Manber's paper
http://web.archive.org/web/20130203031412/http://www.inf.fu-berlin.de/inst/a... There is more recent work also.
Does anyone know whether there's a multiple pattern algorithm for Pharo? One could always fall back on ternary search trees...
On Fri, 14 Dec 2018 at 05:13, Roelof Wobben <r.wobben@home.nl> wrote:
Hello,
I have this code :
FindAndDeleteWordsWithForbiddenParts "deletes all the words that contain forbidden parts"
| result | result := words select: [ :word | [ (forbiddenWords reject: [ :forbidden | '*' , forbidden , '*' match: word ]) isNotEmpty ] on: NotFound do: [ false ] ]. ^ result
but I see warnings that I have to use a stream instead of string concentation.
Anyone hints how to do so ?
Roelof
participants (4)
-
phil@highoctane.be -
Richard O'Keefe -
Richard Sargent -
Roelof Wobben