Where to add methods?

I've a feeling this question is really stupid, so I'll ask the select audience here rather than c.l.s :-)

I'm working on a newsreader. Very, very, early stages for this, and for my real experience of Smalltalk, and
this is a beginner's question about Smalltalk

When reading an article,  the headers are at the start of the article,  terminated by a blank line. If a line within
the headers starts with white space, then it's a continuation of the line above. So there's a test something
like (with currentLine being  a String):

[ currentLine isEmpty not and: [ currentLine first isWhiteSpace ]] whileTrue:
[ "append the next line to the current header, get another currentLine"]

The details aren't important, though, it's the ugly condition I don't like, and I'd like to say something like

[ currentLine isContinuationLine ] whileTrue: [ "as before" ]

But is this adding a method to String (stylistically) OK? It would seem to mean class-browsing common core
library classes would mean reading through lots of methods used elsewhere, and accidental name clashes
seem likely.

Looking through some Squeak source for their mail reader, they seem to do things like (if my class is
UsenetArticle):

   UsenetArticle isContinuationLine: currentLine

which seems even worse than my idea.

So, have I missed something really, really obvious?

P.

Paul,

> The details aren't important, though, it's the ugly condition I don't
like, and I'd like to say something like
>
> [ currentLine isContinuationLine ] whileTrue: [ "as before" ]
>
> But is this adding a method to String (stylistically) OK? It would seem to
mean class-browsing common core
> library classes would mean reading through lots of methods used elsewhere,
and accidental name clashes
> seem likely.

The ability to do this kind of thing is a big advantage of Smalltalk.  If I
were going to add such a metod to String, I'd probably name it something
like #isNntpContinuationLine to make collisions less likley.  You would add
the method as a "loose method" in your NNTP package (so it can't be too bad,
because the package manager is "ready for it"<g>).

That said, I agree that there probably are better ways to solve your current
problem.  You might consider subclassing ReadStream to make an
NNTPReadStream - not sure I like that one very much????  You could view it
as a state machine and follow the State pattern.  If the protocol is simple,
that might be overkill.  Beck describes a Parsing Stream pattern (see Best
Practice Patterns), which basically says to create another object that
captures the "real" stream in an instance variable, and has methods that all
read from it w/o having pass it around as a parameter.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

"Paul Hudson" <[hidden email]> wrote in message
news:Xns9160DF02BE6D5phudsonpoboxcom@127.0.0.1...
[]
> So, have I missed something really, really obvious?

Not really, it's a subject that has no definite answer.

Normally I would (try to) only add methods to a class like String if they
were of a general purpose and didn't need knowledge of any other domain. So,
as a simple example, if I wanted to strip any quote characters off of the
beginning and end of a String then I would add the method to String and call
it #trimQuotes. Implementing a method like #isContinuationLine implies some
extra knowledge - i.e. what is a continuation line - and would probably be
better suited to a class in a domain that could answer that question.

There are lots of occasions when you can't do this though (see the image for
examples), where there is nowhere else to put a method, where the methods
need to access private information about the receiver object or where it
just doesn't "feel right", so you can only treat the above as a guideline.

How would I do it? I would probably have a class called NewsArticle, which
would contain information about a specific article. Part of the instance
creation for the class would be parsing the header information from a
Stream. I would implement a method called #nextHeaderLineFrom: -

nextHeaderLineFrom: aStream
    "Answer the next header line from aStream, allowing for headers
    that are defined over or more lines. Answers an empty string if there
    are no more header lines"

    | nextLine |
    nextLine := aStream nextLine.
    nextLine isEmpty ifTrue: [^nextLine].

    [aStream peek isSpace] whileTrue: [
        nextLine := nextLine , aStream nextLine "trimBlanks?"].

    ^nextLine

Completely untested you understand!! (it probably ought to check for end of
stream as well).  It seems to me to put the code in the correct class, one
that knows about headers and keeps the work in one method with a reasonably
intelligible selector. Your "main" code could then read something like

"["
headerLine := self nextHeaderLineFrom: aStream.
headerLine isEmpty
    ifTrue: [^headers]
    ifFalse: [headers add: (self parseHeaderLine: headerLine)]
"] repeat"

which is readable (?) and all at a similar level of abstraction, another
recommendation for methods.

My Euro0.02 anyway.

Regards
    Ian

"Bill Schwab" <[hidden email]> wrote in
news:9ti78q$nap$[hidden email]:

> That said, I agree that there probably are better ways to solve your
> current problem.  You might consider subclassing ReadStream to make an
> NNTPReadStream - not sure I like that one very much????  

I think I'm with you there :-)

You could
> view it as a state machine and follow the State pattern.  If the
> protocol is simple, that might be overkill.  

... and here...

> Beck describes a Parsing
> Stream pattern (see Best Practice Patterns), which basically says to
> create another object that captures the "real" stream in an instance
> variable, and has methods that all read from it w/o having pass it
> around as a parameter.

I like this one best. I think I should have an NNTPArticleSource class or something, with Ian's nextHeaderLine
method, and create that from aStream.

And I really need to add Best Practice Patterns to my bookshelf (I'e got Guide to Better Smalltalk).

christmasList add: 'Best Practice Patterns'.

Thanks to all of you (Bill, Chris and Ian) for your advice. I'm sure there'll be more questions!

Something like an RFC822 (now RFC2822) parser would be quite widely useful, I think. Squeak contains at
least three, none of which I like very much.

P.

"Ian Bartholomew" <[hidden email]> wrote in
news:6aaL7.7143$h4.444936@stones:

> There are lots of occasions when you can't do this though (see the
> image for examples), where there is nowhere else to put a method,
> where the methods need to access private information about the
> receiver object or where it just doesn't "feel right", so you can only
> treat the above as a guideline.

Right, I'm in "doesn't feel right", I think.

> How would I do it? I would probably have a class called NewsArticle,
> which would contain information about a specific article. Part of the
> instance creation for the class would be parsing the header
> information from a Stream.

Cool. That's exactly what I was doing :-)

I would implement a method called
> #nextHeaderLineFrom: -
>
> nextHeaderLineFrom: aStream
>     "Answer the next header line from aStream, allowing for headers
>     that are defined over or more lines. Answers an empty string if
>     there are no more header lines"

> Completely untested you understand!! (it probably ought to check for
> end of stream as well).  It seems to me to put the code in the correct
> class, one that knows about headers and keeps the work in one method
> with a reasonably intelligible selector.

My problem with this is that it might as well be a class method (no dependency on the object), but it's only
loosely connected with the class, so that feels like just a place to stuff a method to avoid putting it in Stream.

I'm probably over-reacting to the squeak code where the nextLine method in the mail reader is called from
methods of the MailMessage class but implemented in the MailDatabase class...

If you're reading this because of my comment in my reply to Chris, then I meant Bill's reply, not Ian's :-( See
my *other* other reply....

P.

Paul Hudson wrote:

> I'm working on a newsreader.

Not, by any chance, motivated by reading c.l.s and needing an
industrial-grade filtering system ?

> When reading an article,  the headers are at the start of the article,
terminated by a blank line. If a line within
> But is this adding a method to String (stylistically) OK? It would seem to
mean class-browsing
> common core library classes would mean reading through lots of methods
used elsewhere

I'm not as fond of scattering code around the system classes as some people
on c.l.s.  Actually it doesn't seem to be as popular a pass-time among
Dolphin users (possibly because the Dolphin class base is evolving faster
than the older dialects; possbly too because the Dolphin Package mechanism
doesn't deal very well with name clashes).  But whether you do it is between
you and your God.

I would say, though, that *if* you intend your method to live in other
people's images (or even your own when you aren't working on the newreader)
then it's probably not very well named.  #isContinuationLine either means
nothing, or is misleading, outside the context of an RFC822 (or whatever the
number is) header.  So I'd suggest calling it something more helpful, like
#isRFC822ContinuationLine.

The alternative would be to name it my what it *does*, not by what it's
intended to *mean*, in which case #beginsWithWhitespace would do.  In that
case the meaning would be supplied by the place you called it, but I'm not
sure there's *that* much advantage in:

    currentLine beginsWithWhitespace whileTrue: [...]

over

    [currentLine notEmpty and: [currentLine first isWhitespace]] whileTrue:
[...]

FWIW, what I'd probably do is keep the code in my own classes, but wrap up
all the ugly parsing code (and parsing code is *always* ugly) in a class
like RFC822HeaderParser, and just forget it.

> P.

    -- chris

"Chris Uppal" <[hidden email]> wrote in
news:[hidden email]:
>> I'm working on a newsreader.
>
> Not, by any chance, motivated by reading c.l.s and needing an
> industrial-grade filtering system ?

Bingo! Give that man a cigar. Actually, Xnews does pretty well, except that I want a filter rule to kill all posts
by a user - and any responses to any posts by that user (so there's a rule added per match to the poster's
name).

Also, I'm in my first job that doesn't involve hands-on coding at all, and I'm missing it a bit :)
 
> I would say, though, that *if* you intend your method to live in other
> people's images (or even your own when you aren't working on the
> newreader) then it's probably not very well named.
> #isContinuationLine either means nothing, or is misleading, outside
> the context of an RFC822 (or whatever the number is) header.  So I'd
> suggest calling it something more helpful, like
> #isRFC822ContinuationLine.

OK, but I think I should put it in a class where the context does make it clear (or one ends up with the class
name embedded in every method name...)
 
> The alternative would be to name it my what it *does*, not by what
> it's intended to *mean*, in which case #beginsWithWhitespace would do.

>     [currentLine notEmpty and: [currentLine first isWhitespace]]

Until I found isWhiteSpace it was a lot longer!

> FWIW, what I'd probably do is keep the code in my own classes, but
> wrap up all the ugly parsing code (and parsing code is *always* ugly)
> in a class like RFC822HeaderParser, and just forget it.

Right, but I'd still like that to be reasonably neat. I think Ian's got an interesting idea (see my other reply).